GH-3891 - Configure quarantineMode #6073

rob4629 · 2021-03-22T21:48:04Z

Purpose

Describe the problem you want to address or the feature you want to implement.
Closes #3891 : Allow for Quarantine threshold values to be configured (instead of hardcoded to 3). This should include the number of retries, as well as the minimum number of passes.

Approach

Describe how your changes address the issue or implement the desired functionality in as much detail as possible.
I amended quarantineMode to optionally accept a Dictionary. Parse the input (and validate), then use this to set the threshold values of failedThresholdReached and passedThresholdReached

References

Provide a link to the existing issue(s), if any.
#3891

Pre-Merge TODO

Write tests for your proposed changes
Make sure that existing tests do not fail

src/runner/test-run-controller.ts

testcafe-build-bot · 2021-03-22T22:48:42Z

❌ Tests for the commit 23bdc07 have failed. See details:

testcafe-build-bot · 2021-03-24T22:05:40Z

❌ Tests for the commit 8ca6db8 have failed. See details:

testcafe-build-bot · 2021-03-25T00:45:54Z

❌ Tests for the commit 96a77b9 have failed. See details:

miherlosev · 2021-03-25T08:45:59Z

Hi @rob4629
Thank you for contributing to TestCafe. You expose internal failedThreshold and passedThreshold parameters via quarantineMode { "passCount": 1, "retryCount": 5 } API.
Description of parameters:

passCount is the number of passed test runs to consider the test passed
retryCount is the number of failed test runs to consider the test failed

In general, this approach is good.
At the same time, we believe that the retryCount parameter should be a full test run count that TestCafe will execute.
For example:

consider the test passed if at least one run passed - quarantineMode { "passCount": 1 }
increase the number of test run attempts - `quarantineMode { "retryCount": 10 }

This approach doesn't seem to require a lot of changes. You need to calculate the failedThreshold value as a result of the retryCount minus passCount expression and validate if that passCount is less than retryCount.

rob4629 · 2021-03-25T16:47:24Z

Thanks for the feedback @miherlosev. I can implement that change today. Does passCount seem a sensible/explicit name to you, or should this be passThreshold?

Also, it looks like my change has broken existing functionality:

testcafe -q tests/myFixture/myTest.js

ERROR The "--quarantine-mode" option value is not a valid key-value pair.

If I explicitly add the testpath argument afterwards, it behaves as expected

testcafe -q -t myFixture

Although I'm validating it's a valid Key-Value pair in getQuarantineOptions, should I add some sort of filtering/conditions to _parseQuarantineOptions?

rob4629 · 2021-03-25T23:04:12Z

This approach doesn't seem to require a lot of changes. You need to calculate the failedThreshold value as a result of the retryCount minus passCount expression and validate if that passCount is less than retryCount.

Ok, I don't think I understand your suggestion... here's how it sounds to me (pseudocode)

const failedThreshold = passCount < retryCount ? retryCount - passCount : DEFAULT_QUARANTINE_THRESHOLD;

const failedThresholdReached = failedTimes >= failedThreshold;
....

testcafe -q                                // no change
testcafe -q passCount=1                    // failedThreshold = 2
testcafe -q retryCount=5                   // failedThreshold = 2
testcafe -q retryCount=10                  // failedThreshold = 7
testcafe -q retryCount=10,passCount=1      // failedThreshold = 9

Is that your expectation?

miherlosev · 2021-03-29T13:59:46Z

In general, you understand the explanation correctly.
Just one correction: you should validate that the passCount is less than retryCount.

Pseudocode

validateArguments({ retryCount, passCount });

const failedThreshold = retryCount - passCount;

const failedThresholdReached = failedTimes >= failedThreshold;

// All correct
testcafe -q                                // no change
testcafe -q passCount=1                    // failedThreshold = 2
testcafe -q retryCount=5                   // failedThreshold = 2
testcafe -q retryCount=10                  // failedThreshold = 7
testcafe -q retryCount=10,passCount=1      // failedThreshold = 9

// Raise an error about incorrect parameters
testcafe -q retryCount=3,passCount=4

…ount

rob4629 · 2021-03-29T19:14:05Z

Ok, I've added further validation to the argument-parser, plus 2 new tests to verify passCount < retryCount.

I've also implemented the failedThreshold as per your request, however I believe we should have a minimum failedThreshold of 3, rather than setting failedThreshold to 1 for -q retryCount=4 or -q passCount=1.

Ultimately the decision is yours, but I look forward to hearing your thoughts.

testcafe-build-bot · 2021-04-15T16:18:43Z

❌ Tests for the commit fb95e3a have failed. See details:

testcafe-build-bot · 2021-04-16T09:40:58Z

❌ Tests for the commit fb95e3a have failed. See details:

rob4629 · 2021-04-22T05:02:29Z

Do you need me to do more for this PR? It seems like my local master branch has become corrupted... So I'm trying to resolve that before I can investigate these failing tests

DIRECTcut · 2021-04-22T09:31:03Z

@rob4629 Great PR.

We've moved our documentation to a separate repository and it's not available for open-source contributions anymore. Please rebase your PR and remove all documentation changes (changes to .md files) from this PR.

I'll make sure that the documentation for this new feature makes it to our website.

Thanks.

testcafe-build-bot · 2021-04-22T23:02:47Z