feat: add lastFailed config option #973

Kabir-Ivan · 2024-07-16T13:57:01Z

Make it possible to run only failed tests by adding --run-failed option in CLI.

KuznetsovRoman

Otherwise, /ok
But let's see what the others say

src/testplane.ts

shadowusr · 2024-07-17T08:58:14Z

docs/config.md

@@ -593,6 +594,9 @@ export const {
 }
 ```

+### failedTestsPath
+Testplane will save a list of all failed tests to `failedTestsPath`


Would be nice to specify the default value here.

src/testplane.ts

shadowusr · 2024-07-17T11:07:56Z

src/cli/index.js

@@ -58,6 +58,7 @@ exports.run = (opts = {}) => {
            "--update-refs",
            'update screenshot references or gather them if they do not exist ("assertView" command)',
        )
+        .option("--run-failed", "only run tests that failed during the last run")


I think it may be named something like --run-last-failed, that would sound more clear

shadowusr · 2024-07-17T11:09:27Z

src/cli/index.js

@@ -78,6 +79,7 @@ exports.run = (opts = {}) => {
                    set: sets,
                    grep,
                    updateRefs,
+                    runFailed,


Also, perhaps there should be an option in config and env variable for that as well? I'm not sure though. Thinking about using this in CI, it would be convenient to have separate config for CI to run only failed tests by default or specify it by using env variable. Discussble

shadowusr · 2024-07-17T11:21:15Z

src/testplane.ts

+    }
+
+    protected async _saveFailed(): Promise<void> {
+        const dirname = path.dirname(this._config.failedTestsPath);


Thinking about using this in CI, I think it would be convenient to be able to fetch this file from remote URL...

But that'd require to have two separate options for input and output. Generally, let's discuss with the team if we need run options like grep and files to be in config?
This would make possible doing so:

export default { grep: /some-test/, lastFailed: { only: true, output: '.testplane/file.json', input: 'https://ci-artifacts.com/.../file.json', }, // or just true/false if default input/output are sufficient // other config options }

shadowusr · 2024-07-17T11:23:20Z

src/testplane.ts

+            });
+
+            this.on(MasterEvents.AFTER_TESTS_READ, testCollection => {
+                if (previousRunFailedTests.length) {


I'm not sure if this is the best way to do this. In src/test-reader/test-parser we have this code:

if (grep) { treeBuilder.addTestFilter(test => grep.test(test.fullTitle())); }

We probably can implement this the same way, by adding a filter function.

The goal is to keep main file src/testplane.ts short and clean.

shadowusr · 2024-07-17T11:24:48Z

src/testplane.ts

+                    previousRunFailedTests.forEach(({ fullTitle, browserId }) => {
+                        testCollection.enableTest(fullTitle, browserId);
+                    });
+                }


Also, if run-failed option was passed, but no failed tests file was found, should we print a warning about it, what do you think?

DudaGod

There are some questions

docs/config.md

src/config/types.ts

src/testplane.ts

test/src/testplane.js

test/src/test-reader/test-parser.js

test/src/testplane.js

DudaGod

Everything is cool, but I suggest using Map for matching and add browserVersion to failedTests, because we can use few versions of one browser.

DudaGod · 2024-07-22T11:05:43Z

src/test-reader/test-parser.js

+                this.#failedTests = await fs.readJSON(config.lastFailed.input);
+            } catch {
+                logger.warn(
+                    `Could not read failed tests data at ${this._config.lastFailed.input}. Running all tests instead`,


To my mind we should write an error here, because of which we couldn't read the file.

DudaGod · 2024-07-22T11:52:19Z

src/test-reader/test-parser.js

@@ -66,6 +70,16 @@ class TestParser extends EventEmitter {
        const esmDecorator = f => f + `?rand=${rand}`;
        await readFiles(files, { esmDecorator, config: mochaOpts, eventBus });

+        if (config.lastFailed.only) {
+            try {
+                this.#failedTests = await fs.readJSON(config.lastFailed.input);


Can we store tests like object with uniq id as string? It gives ability to match tests by key faster then iterate by array.

Moreover we should save browserVersion because we have pluign like https://github.com/gemini-testing/hermione-browser-version-changer/tree/master which gives ability to run tests in different browser versions. More info here - https://nda.ya.ru/t/ctpevAUg777hZX.

So to my mind we can use Map like this:

#failedTests: Map<string, {fullTitle: string, browserId: string, browserVersion?: string}>; // ... this.#failedTests = new Map(); for (test of await fs.readJSON(config.lastFailed.input)) { const id = getShortMD5(`${test.fullTitle}${test.browserId}${test.browserVersion}`); this.#failedTests.set(id, _.pick(test, ['fullTitle', 'browserId', 'browserVersion']); });

DudaGod · 2024-07-22T12:06:20Z

src/test-reader/test-parser.js

+            const failedStringified = this.#failedTests.map(data => JSON.stringify(data));
+            treeBuilder.addTestFilter(test =>
+                failedStringified.includes(
+                    JSON.stringify({


I don't like solution with iterate through all tests in order to find desired one. I described the above solution with Map, which should be much faster. What do you think?

It will also allow us to get rid of json.strigify here.

DudaGod

The source is ok, but I don't see a lot of tests on this new functionality. Let's add them.

DudaGod · 2024-07-25T08:43:51Z

src/test-reader/test-parser.js

+                    : config.lastFailed.input.split(",").map(v => v.trim());
+                for (const inputPath of inputPaths) {
+                    for (const test of await fs.readJSON(inputPath)) {
+                        this.#failedTests.add(getShortMD5(`${test.fullTitle}${test.browserId}${test.browserVersion}`));


Let's maybe put it in a separate function? Like this:

const getFailedTestId = (test) => getShortMD5(`${test.fullTitle}${test.browserId}${test.browserVersion}`);

DudaGod · 2024-07-25T08:47:39Z

test/src/config/options.js

+
+                Config.read.returns(readConfig);
+
+                assert.throws(() => createConfig(), Error, '"lastFailed.input" must be a string');


Error message was changed

DudaGod · 2024-07-25T08:48:01Z

test/src/config/options.js

+            });
+        });
+
+        describe("input", () => {


I don't see tests:

string without .json at the end -> throw

array without string + .json at the end -> throw

string with .json -> ok

array with .json -> ok

DudaGod · 2024-07-25T08:48:38Z

test/src/config/options.js

+
+                Config.read.returns(readConfig);
+
+                assert.throws(() => createConfig(), Error, '"lastFailed.output" must be a string');


Error message was changed

DudaGod · 2024-07-25T08:49:01Z

test/src/config/options.js

+            });
+        });
+
+        describe("output", () => {


I don't see tests:

string without .json at the end -> throw

string with .json -> ok

DudaGod · 2024-07-25T09:30:43Z

test/src/test-reader/test-parser.js

@@ -14,6 +14,7 @@ const proxyquire = require("proxyquire").noCallThru();
 const path = require("path");


Where are the tests for this module? There should be quite a lot of them here since the main code is in this file.

In loadFiles (public) we can check that:

config.lastFailed.only not set -> do not read json files

config.lastFailed.only not set -> do not call addTestFilter (parse should be called after loadFiles)

config.lastFailed.only set -> read json from one file (string)

config.lastFailed.only set -> read json from one file (string with commas, case with env)

config.lastFailed.only set -> read json from few files (array of strings)

and other tests

Method parse must be tested as well.

DudaGod · 2024-07-25T09:32:02Z

test/src/testplane.js


                return runTestplane().then(success => assert.isFalse(success));
            });

-            it("should halt if there were some errors", () => {


Why you remove this test?

DudaGod · 2024-07-25T09:34:11Z

test/src/testplane.js

@@ -467,16 +497,20 @@ describe("testplane", () => {
            it("all runner events with passed event data", () => {
                const runner = mkNodejsEnvRunner_();
                const testplane = mkTestplane_();
+                const results = {
+                    fullTitle: () => "Title",
+                    browserId: "chrome",


then maybe add browserVersion as well?

DudaGod · 2024-07-25T09:34:15Z

test/src/testplane.js

@@ -761,8 +795,13 @@ describe("testplane", () => {
        it('should return "true" after some test fail', () => {
            const testplane = mkTestplane_();

+            const results = {
+                fullTitle: () => "Title",
+                browserId: "chrome",


then maybe add browserVersion as well?

DudaGod

I suggest fix few tests, but otherwise everything is ok!

DudaGod · 2024-07-29T08:19:45Z

src/test-reader/test-parser.js

        this.#buildInstructions = new InstructionsList();
    }

+    getFailedTestId = test => getShortMD5(`${test.fullTitle}${test.browserId}${test.browserVersion}`);


It should be a helper, because it doesn't use class methods

DudaGod · 2024-07-29T08:33:49Z

test/src/test-reader/test-parser.js

+
+                await parse_({ config }, config);
+
+                assert.calledOnce(TreeBuilder.prototype.addTestFilter);


We can get callback using - TreeBuilder.prototype.addTestFilter.lastCall.args[0] and call it with passed test. And then check if it returns true or false. You can see examples of testing grep in this file.

KuznetsovRoman approved these changes Jul 17, 2024

View reviewed changes

src/testplane.ts Outdated Show resolved Hide resolved

src/testplane.ts Outdated Show resolved Hide resolved

src/testplane.ts Outdated Show resolved Hide resolved

src/testplane.ts Outdated Show resolved Hide resolved

src/testplane.ts Outdated Show resolved Hide resolved

KuznetsovRoman reviewed Jul 17, 2024

View reviewed changes

src/testplane.ts Outdated Show resolved Hide resolved

src/testplane.ts Outdated Show resolved Hide resolved

src/testplane.ts Outdated Show resolved Hide resolved

src/testplane.ts Outdated Show resolved Hide resolved

src/testplane.ts Outdated Show resolved Hide resolved

shadowusr requested changes Jul 17, 2024

View reviewed changes

DudaGod requested changes Jul 19, 2024

View reviewed changes

DudaGod requested changes Jul 22, 2024

View reviewed changes

DudaGod requested changes Jul 25, 2024

View reviewed changes

Kabir-Ivan requested a review from DudaGod July 25, 2024 12:52

DudaGod approved these changes Jul 29, 2024

View reviewed changes

Kabir-Ivan added 12 commits July 29, 2024 14:04

feat: add --run-failed option

fb29d8b

refactor: refactor using fs-extra

945378f

refactor: use outputJSON from fs-extra

86a1d9a

feat: implement lastFailed config option

a2b07da

refactor: remove unused code

d45a4d0

refactor: refactor code

bd9176d

refactor: add browser version

ac41831

feat: allow arrays in lastFailed input

c6ee901

fix: fix bugs

fb8c50d

test: add tests

f15ac17

refactor: minor fixes

709514d

refactor: minor changes

b85ef02

Kabir-Ivan force-pushed the add-run-failed branch from deee61f to b85ef02 Compare July 29, 2024 11:04

Kabir-Ivan merged commit 36aa516 into master Jul 29, 2024
2 checks passed

Kabir-Ivan deleted the add-run-failed branch July 29, 2024 11:06

Kabir-Ivan changed the title ~~feat: add --run-failed option~~ feat: add lastFailed config option Aug 15, 2024

Kabir-Ivan mentioned this pull request Aug 15, 2024

Add ability to rerun all failed tests #953

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add lastFailed config option #973

feat: add lastFailed config option #973

Kabir-Ivan commented Jul 16, 2024 •

edited

Loading

KuznetsovRoman left a comment

shadowusr Jul 17, 2024

shadowusr Jul 17, 2024

shadowusr Jul 17, 2024

shadowusr Jul 17, 2024

shadowusr Jul 17, 2024 •

edited

Loading

shadowusr Jul 17, 2024

DudaGod left a comment

DudaGod left a comment

DudaGod Jul 22, 2024

DudaGod Jul 22, 2024

DudaGod Jul 22, 2024

DudaGod left a comment

DudaGod Jul 25, 2024

DudaGod Jul 25, 2024

DudaGod Jul 25, 2024

DudaGod Jul 25, 2024

DudaGod Jul 25, 2024

DudaGod Jul 25, 2024

DudaGod Jul 25, 2024

DudaGod Jul 25, 2024

DudaGod Jul 25, 2024

DudaGod left a comment

DudaGod Jul 29, 2024

DudaGod Jul 29, 2024


		Config.read.returns(readConfig);

		assert.throws(() => createConfig(), Error, '"lastFailed.input" must be a string');

		@@ -14,6 +14,7 @@ const proxyquire = require("proxyquire").noCallThru();
		const path = require("path");


		await parse_({ config }, config);

		assert.calledOnce(TreeBuilder.prototype.addTestFilter);

feat: add lastFailed config option #973

feat: add lastFailed config option #973

Conversation

Kabir-Ivan commented Jul 16, 2024 • edited Loading

KuznetsovRoman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadowusr Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DudaGod left a comment

Choose a reason for hiding this comment

DudaGod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DudaGod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DudaGod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kabir-Ivan commented Jul 16, 2024 •

edited

Loading

shadowusr Jul 17, 2024 •

edited

Loading