[ci-visibility] Early flake detection for jest #3956
Conversation
Overall package sizeSelf size: 6 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3956 +/- ##
==========================================
+ Coverage 83.92% 85.15% +1.23%
==========================================
Files 107 243 +136
Lines 4330 10504 +6174
Branches 33 33
==========================================
+ Hits 3634 8945 +5311
- Misses 696 1559 +863 ☔ View full report in Codecov by Sentry. |
BenchmarksBenchmark execution time: 2024-02-07 14:18:39 Comparing candidate commit 6460898 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 259 metrics, 6 unstable metrics. scenario:plugin-graphql-with-depth-on-max-18
|
juan-fernandez
force-pushed
the
juan-fernandez/early-flake-detection
branch
2 times, most recently
from
6302b85 to
eb96dd4
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
juan-fernandez
force-pushed
the
juan-fernandez/early-flake-detection
branch
from
2208e26 to
45662e0
Compare
juan-fernandez
force-pushed
the
juan-fernandez/early-flake-detection
branch
2 times, most recently
from
da8ee90 to
61c3e1a
Compare
juan-fernandez
changed the title
| ...jestCommonOptions, | ||
| testFile: 'ci-visibility/run-jest.mjs', | ||
| runTestsWithCoverageCommand: `node --loader=${hookFile} ./ci-visibility/run-jest.mjs`, | ||
| type: 'esm' |
There was a problem hiding this comment.
esm tests were being skipped altogether. Until we add them back this is noise, so I'm removing it
| @@ -676,9 +676,7 @@ describe('CI Visibility Exporter', () => { | |||
| ciVisibilityExporter._resolveCanUseCiVisProtocol(false) | |||
| ciVisibilityExporter._libraryConfig = { isEarlyFlakeDetectionEnabled: true } | |||
| ciVisibilityExporter.getKnownTests({}, (err) => { | |||
| expect(err.message).to.include( | |||
| 'Known tests can not be requested because CI Visibility protocol can not be used' | |||
There was a problem hiding this comment.
no need to do this: if CI Vis protocol can't be used, the library settings can't be fetched, so we will not even reach this situation
juan-fernandez
force-pushed
the
juan-fernandez/early-flake-detection
branch
from
34b76b3 to
89fd074
Compare
juan-fernandez
force-pushed
the
juan-fernandez/early-flake-detection
branch
from
89fd074 to
d4d6eec
Compare
| // Add tests to be retried to the list of tests to run | ||
| for (let retryIndex = 0; retryIndex < earlyFlakeDetectionNumRetries; retryIndex++) { | ||
| if (this.global.test) { | ||
| this.global.test(getEfdTestName(event.testName, retryIndex), event.fn, event.timeout) |
There was a problem hiding this comment.
this is the hacky part: if we detect that the test is new, we'll call this.global.test right here, which is like defining a new test test('...')
| @@ -62,6 +68,7 @@ const specStatusToTestStatus = { | |||
|
|
|||
| const asyncResources = new WeakMap() | |||
| const originalTestFns = new WeakMap() | |||
| const retriedTestsToNumAttempts = new Map() | |||
There was a problem hiding this comment.
retriedTestsToNumAttempts is used to track how many times a test has been retried
| @@ -90,6 +97,14 @@ function getTestEnvironmentOptions (config) { | |||
| return {} | |||
| } | |||
|
|
|||
| function getEfdTestName (testName, numAttempt) { | |||
| return `${EFD_STRING} (#${numAttempt}): ${testName}` | |||
There was a problem hiding this comment.
to make it as explicit as possible why the test is being retried
| } = remoteConfiguration | ||
| return { | ||
| isCodeCoverageEnabled, | ||
| isSuitesSkippingEnabled, | ||
| isItrEnabled, | ||
| requireGit, | ||
| isEarlyFlakeDetectionEnabled: isEarlyFlakeDetectionEnabled && this._config.isEarlyFlakeDetectionEnabled | ||
| isEarlyFlakeDetectionEnabled: isEarlyFlakeDetectionEnabled && this._config.isEarlyFlakeDetectionEnabled, | ||
| earlyFlakeDetectionNumRetries |
There was a problem hiding this comment.
improvements unrelated to the feature in this file
| @@ -14,7 +14,10 @@ const { | |||
| TEST_ITR_UNSKIPPABLE, | |||
| TEST_ITR_FORCED_RUN, | |||
| TEST_CODE_OWNERS, | |||
| ITR_CORRELATION_ID | |||
| ITR_CORRELATION_ID, | |||
| TEST_IS_NEW, | |||
There was a problem hiding this comment.
Is the concept of a new test meaningful beyond EFD? I'm tempted to suggest this should be TEST_EARLY_FLAKE_IS_NEW .
There was a problem hiding this comment.
I'd say so. We've had issues in the past where we've namespaced tags only to realise that they were more general. I think that's a bigger problem than having a "general" tag only being applied to a feature / subproduct (but I don't even think that's going to be an issue here)
| @@ -11,6 +11,8 @@ const { | |||
| getErrorTypeFromStatusCode | |||
| } = require('../telemetry') | |||
|
|
|||
| const DEFAULT_NUM_RETRIES_EARLY_FLAKE_DETECTION = 2 | |||
There was a problem hiding this comment.
nit: DEFAULT_EARLY_FLAKE_DETECTION_NUM_RETRIES would match earlyFlakeDetectionNumRetries better.
There was a problem hiding this comment.
What does this PR do?
Add early flake detection to
jest.The way it works:
add_testevent), we check whether it shows up in our list of known tests.this.global.test, which is the function the user calls to define tests).Motivation
Early flake detection's objective is to detect flakiness before it reaches the main branch. Most of the flakiness from a repository comes from undetected flaky tests that are added to the repository (vs a smaller part which comes from modifications of already existing tests).
By knowing what tests a given test service has run, we can detect which ones are new (that is, being added in the current branch), and run them multiple times. By running them multiple times we can surface flakiness, should there be any.
Plugin Checklist
Security
Datadog employees:
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!