From 3ec51b3ce8a1c6d3c1e56c42a93dae4788a2f08c Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 25 Sep 2023 10:43:29 -0400 Subject: [PATCH] chore: address comments from code review --- .../e2e/util/mocha_custom_methods.cy.js | 44 +++++++++---------- packages/driver/src/cypress/mocha.ts | 38 +++++++++------- packages/driver/src/cypress/runner.ts | 8 ++-- 3 files changed, 49 insertions(+), 41 deletions(-) diff --git a/packages/driver/cypress/e2e/util/mocha_custom_methods.cy.js b/packages/driver/cypress/e2e/util/mocha_custom_methods.cy.js index a10266c0855c..29f9d94e2f98 100644 --- a/packages/driver/cypress/e2e/util/mocha_custom_methods.cy.js +++ b/packages/driver/cypress/e2e/util/mocha_custom_methods.cy.js @@ -21,7 +21,7 @@ describe('mocha custom methods', () => { describe('strategy w/ experimentalBurnIn=false', () => { it('should never attempt to retry a test that passes on the first try, regardless of strategy', function () { const undefinedStrategyTest = createMockTest() - const noExperimentalRetries = calculateTestStatus.call(undefinedStrategyTest) + const noExperimentalRetries = calculateTestStatus(undefinedStrategyTest) expect(noExperimentalRetries.outerStatus).to.equal('passed') expect(noExperimentalRetries.attempts).to.equal(1) @@ -30,7 +30,7 @@ describe('mocha custom methods', () => { expect(undefinedStrategyTest.final).to.be.true const detectFlakeAndPassOnThresholdStrategyTest = createMockTest() - const detectFlakeAndPassOnThreshold = calculateTestStatus.call(detectFlakeAndPassOnThresholdStrategyTest, 'detect-flake-and-pass-on-threshold', { + const detectFlakeAndPassOnThreshold = calculateTestStatus(detectFlakeAndPassOnThresholdStrategyTest, 'detect-flake-and-pass-on-threshold', { maxRetries: 8, passesRequired: 5, }) @@ -42,7 +42,7 @@ describe('mocha custom methods', () => { expect(detectFlakeAndPassOnThresholdStrategyTest.final).to.be.true const detectFlakeButAlwaysFailStrategyTest = createMockTest() - const detectFlakeButAlwaysFail = calculateTestStatus.call(detectFlakeButAlwaysFailStrategyTest, 'detect-flake-but-always-fail', { + const detectFlakeButAlwaysFail = calculateTestStatus(detectFlakeButAlwaysFailStrategyTest, 'detect-flake-but-always-fail', { maxRetries: 8, stopIfAnyPassed: false, }) @@ -57,7 +57,7 @@ describe('mocha custom methods', () => { describe('undefined (GA implementation/original)', () => { it('passed: keeps signaling to retry until test passes', function () { const mockTest1 = createMockTest('failed') - const attempt1 = calculateTestStatus.call(mockTest1) + const attempt1 = calculateTestStatus(mockTest1) expect(attempt1.outerStatus).to.be.undefined expect(attempt1.attempts).to.equal(1) @@ -65,7 +65,7 @@ describe('mocha custom methods', () => { expect(attempt1.strategy).to.be.undefined const mockTest2 = createMockTest('passed', [mockTest1]) - const attempt2 = calculateTestStatus.call(mockTest2) + const attempt2 = calculateTestStatus(mockTest2) expect(attempt2.outerStatus).to.equal('passed') expect(attempt2.attempts).to.equal(2) @@ -77,7 +77,7 @@ describe('mocha custom methods', () => { // this happens inside ./driver/src/cypress/runner.ts it('failed: keeps signaling to retry until retry limit is reached', function () { const mockTest1 = createMockTest('failed') - const attempt1 = calculateTestStatus.call(mockTest1) + const attempt1 = calculateTestStatus(mockTest1) expect(attempt1.outerStatus).to.be.undefined expect(attempt1.attempts).to.equal(1) @@ -85,7 +85,7 @@ describe('mocha custom methods', () => { expect(attempt1.strategy).to.be.undefined const mockTest2 = createMockTest('failed', [mockTest1]) - const attempt2 = calculateTestStatus.call(mockTest2) + const attempt2 = calculateTestStatus(mockTest2) expect(attempt2.outerStatus).to.be.undefined expect(attempt2.attempts).to.equal(2) @@ -93,7 +93,7 @@ describe('mocha custom methods', () => { expect(attempt2.strategy).to.be.undefined const mockTest3 = createMockTest('failed', [mockTest1, mockTest2]) - const attempt3 = calculateTestStatus.call(mockTest3) + const attempt3 = calculateTestStatus(mockTest3) expect(attempt3.outerStatus).to.equal('failed') expect(attempt3.attempts).to.equal(3) @@ -106,7 +106,7 @@ describe('mocha custom methods', () => { it('passed: no longer signals to retry test after passesRequired threshold is reached', function () { totalRetries = 5 const mockTest1 = createMockTest('failed') - const attempt1 = calculateTestStatus.call(mockTest1, 'detect-flake-and-pass-on-threshold', { + const attempt1 = calculateTestStatus(mockTest1, 'detect-flake-and-pass-on-threshold', { maxRetries: totalRetries, passesRequired: 2, }) @@ -118,7 +118,7 @@ describe('mocha custom methods', () => { expect(mockTest1.final).to.be.false const mockTest2 = createMockTest('failed', [mockTest1]) - const attempt2 = calculateTestStatus.call(mockTest2, 'detect-flake-and-pass-on-threshold', { + const attempt2 = calculateTestStatus(mockTest2, 'detect-flake-and-pass-on-threshold', { maxRetries: totalRetries, passesRequired: 2, }) @@ -130,7 +130,7 @@ describe('mocha custom methods', () => { expect(mockTest2.final).to.be.false const mockTest3 = createMockTest('passed', [mockTest1, mockTest2]) - const attempt3 = calculateTestStatus.call(mockTest3, 'detect-flake-and-pass-on-threshold', { + const attempt3 = calculateTestStatus(mockTest3, 'detect-flake-and-pass-on-threshold', { maxRetries: totalRetries, passesRequired: 2, }) @@ -142,7 +142,7 @@ describe('mocha custom methods', () => { expect(mockTest3.final).to.be.false const mockTest4 = createMockTest('passed', [mockTest1, mockTest2, mockTest3]) - const attempt4 = calculateTestStatus.call(mockTest4, 'detect-flake-and-pass-on-threshold', { + const attempt4 = calculateTestStatus(mockTest4, 'detect-flake-and-pass-on-threshold', { maxRetries: totalRetries, passesRequired: 2, }) @@ -157,7 +157,7 @@ describe('mocha custom methods', () => { it('failed: no longer signals to retry test if the passesRequired is impossible to meet', function () { totalRetries = 4 const mockTest1 = createMockTest('failed') - const attempt1 = calculateTestStatus.call(mockTest1, 'detect-flake-and-pass-on-threshold', { + const attempt1 = calculateTestStatus(mockTest1, 'detect-flake-and-pass-on-threshold', { maxRetries: totalRetries, passesRequired: 2, }) @@ -169,7 +169,7 @@ describe('mocha custom methods', () => { expect(mockTest1.final).to.be.false const mockTest2 = createMockTest('failed', [mockTest1]) - const attempt2 = calculateTestStatus.call(mockTest2, 'detect-flake-and-pass-on-threshold', { + const attempt2 = calculateTestStatus(mockTest2, 'detect-flake-and-pass-on-threshold', { maxRetries: totalRetries, passesRequired: 2, }) @@ -181,7 +181,7 @@ describe('mocha custom methods', () => { expect(mockTest2.final).to.be.false const mockTest3 = createMockTest('failed', [mockTest1, mockTest2]) - const attempt3 = calculateTestStatus.call(mockTest3, 'detect-flake-and-pass-on-threshold', { + const attempt3 = calculateTestStatus(mockTest3, 'detect-flake-and-pass-on-threshold', { maxRetries: totalRetries, passesRequired: 2, }) @@ -193,7 +193,7 @@ describe('mocha custom methods', () => { expect(mockTest3.final).to.be.false const mockTest4 = createMockTest('failed', [mockTest1, mockTest2, mockTest3]) - const attempt4 = calculateTestStatus.call(mockTest4, 'detect-flake-and-pass-on-threshold', { + const attempt4 = calculateTestStatus(mockTest4, 'detect-flake-and-pass-on-threshold', { maxRetries: totalRetries, passesRequired: 2, }) @@ -210,7 +210,7 @@ describe('mocha custom methods', () => { it('failed: no longer signals to retry after retries are exhausted', function () { totalRetries = 3 const mockTest1 = createMockTest('failed') - const attempt1 = calculateTestStatus.call(mockTest1, 'detect-flake-but-always-fail', { + const attempt1 = calculateTestStatus(mockTest1, 'detect-flake-but-always-fail', { maxRetries: totalRetries, stopIfAnyPassed: false, }) @@ -222,7 +222,7 @@ describe('mocha custom methods', () => { expect(mockTest1.final).to.be.false const mockTest2 = createMockTest('failed', [mockTest1]) - const attempt2 = calculateTestStatus.call(mockTest2, 'detect-flake-but-always-fail', { + const attempt2 = calculateTestStatus(mockTest2, 'detect-flake-but-always-fail', { maxRetries: totalRetries, stopIfAnyPassed: false, }) @@ -234,7 +234,7 @@ describe('mocha custom methods', () => { expect(mockTest2.final).to.be.false const mockTest3 = createMockTest('passed', [mockTest1, mockTest2]) - const attempt3 = calculateTestStatus.call(mockTest3, 'detect-flake-but-always-fail', { + const attempt3 = calculateTestStatus(mockTest3, 'detect-flake-but-always-fail', { maxRetries: totalRetries, stopIfAnyPassed: false, }) @@ -246,7 +246,7 @@ describe('mocha custom methods', () => { expect(mockTest3.final).to.be.false const mockTest4 = createMockTest('passed', [mockTest1, mockTest2, mockTest3]) - const attempt4 = calculateTestStatus.call(mockTest4, 'detect-flake-but-always-fail', { + const attempt4 = calculateTestStatus(mockTest4, 'detect-flake-but-always-fail', { maxRetries: totalRetries, stopIfAnyPassed: false, }) @@ -263,7 +263,7 @@ describe('mocha custom methods', () => { it('failed: short circuits after a retry has a passed test', function () { totalRetries = 3 const mockTest1 = createMockTest('failed') - const attempt1 = calculateTestStatus.call(mockTest1, 'detect-flake-but-always-fail', { + const attempt1 = calculateTestStatus(mockTest1, 'detect-flake-but-always-fail', { maxRetries: totalRetries, stopIfAnyPassed: true, }) @@ -275,7 +275,7 @@ describe('mocha custom methods', () => { expect(mockTest1.final).to.be.false const mockTest2 = createMockTest('passed', [mockTest1]) - const attempt2 = calculateTestStatus.call(mockTest2, 'detect-flake-but-always-fail', { + const attempt2 = calculateTestStatus(mockTest2, 'detect-flake-but-always-fail', { maxRetries: totalRetries, stopIfAnyPassed: true, }) diff --git a/packages/driver/src/cypress/mocha.ts b/packages/driver/src/cypress/mocha.ts index 5666d04a43bb..e1b190870eb1 100644 --- a/packages/driver/src/cypress/mocha.ts +++ b/packages/driver/src/cypress/mocha.ts @@ -38,26 +38,29 @@ delete (window as any).Mocha export const SKIPPED_DUE_TO_BROWSER_MESSAGE = ' (skipped due to browser)' // NOTE: 'calculateTestStatus' is marked as an individual function to make functionality easier to test. -export const calculateTestStatus = function (strategy?: 'detect-flake-and-pass-on-threshold' | 'detect-flake-but-always-fail', options?: any) { - const totalAttemptsAlreadyExecuted = this.currentRetry() + 1 +export function calculateTestStatus (test: Mocha.Test, strategy?: 'detect-flake-and-pass-on-threshold' | 'detect-flake-but-always-fail', options?: any) { + // @ts-expect-error + const totalAttemptsAlreadyExecuted = test.currentRetry() + 1 let shouldAttemptsContinue: boolean = true let outerTestStatus: 'passed' | 'failed' | undefined = undefined - const passedTests = _.filter(this.prevAttempts, (o) => o.state === 'passed') - const failedTests = _.filter(this.prevAttempts, (o) => o.state === 'failed') + // @ts-expect-error + const passedTests = _.filter(test.prevAttempts, (o) => o.state === 'passed') + // @ts-expect-error + const failedTests = _.filter(test.prevAttempts, (o) => o.state === 'failed') // Additionally, if the current test attempt passed/failed, add it to the attempt list - if (this.state === 'passed') { - passedTests.push(this) - } else if (this.state === 'failed') { - failedTests.push(this) + if (test.state === 'passed') { + passedTests.push(test) + } else if (test.state === 'failed') { + failedTests.push(test) } // If there is AT LEAST one failed test attempt, we know we need to apply retry logic. // Otherwise, the test might be burning in (not implemented yet) OR the test passed on the first attempt, // meaning retry logic does NOT need to be applied. if (failedTests.length > 0) { - const maxAttempts = this.retries() + 1 + const maxAttempts = test.retries() + 1 const remainingAttempts = maxAttempts - totalAttemptsAlreadyExecuted const passingAttempts = passedTests.length @@ -78,7 +81,8 @@ export const calculateTestStatus = function (strategy?: 'detect-flake-and-pass-o // Do we have the required amount of passes? If yes, we no longer need to keep running the test. if (strategy !== 'detect-flake-but-always-fail' && passingAttempts >= passesRequired) { outerTestStatus = 'passed' - this.final = true + // @ts-expect-error + test.final = true shouldAttemptsContinue = false } else if (totalAttemptsAlreadyExecuted < maxAttempts && ( @@ -94,22 +98,26 @@ export const calculateTestStatus = function (strategy?: 'detect-flake-and-pass-o // retry the test. (strategy === 'detect-flake-but-always-fail' && (!stopIfAnyPassed || stopIfAnyPassed && passingAttempts === 0)) )) { - this.final = false + // @ts-expect-error + test.final = false shouldAttemptsContinue = true } else { // Otherwise, we should stop retrying the test. outerTestStatus = 'failed' - this.final = true + // @ts-expect-error + test.final = true // If an outerStatus is 'failed', but the last test attempt was 'passed', we need to force the status so mocha doesn't flag the test attempt as failed. // This is a common use case with 'detect-flake-but-always-fail', where we want to display the last attempt as 'passed' but fail the test. - this.forceState = this.state === 'passed' ? this.state : undefined + // @ts-expect-error + test.forceState = test.state === 'passed' ? test.state : undefined shouldAttemptsContinue = false } } else { // retry logic did not need to be applied and the test passed. outerTestStatus = 'passed' shouldAttemptsContinue = false - this.final = true + // @ts-expect-error + test.final = true } return { @@ -428,7 +436,7 @@ function createCalculateTestStatus (Cypress: Cypress.Cypress) { let retriesConfig = Cypress.config('retries') // @ts-expect-error - return calculateTestStatus.call(this, retriesConfig?.experimentalStrategy, retriesConfig?.experimentalOptions) + return calculateTestStatus(this, retriesConfig?.experimentalStrategy, retriesConfig?.experimentalOptions) } } diff --git a/packages/driver/src/cypress/runner.ts b/packages/driver/src/cypress/runner.ts index 81938054b7dc..6dddb4275bd3 100644 --- a/packages/driver/src/cypress/runner.ts +++ b/packages/driver/src/cypress/runner.ts @@ -482,16 +482,16 @@ const overrideRunnerHook = (Cypress, _runner, getTestById, getTest, setTest, get test.final = true if (test.state === 'passed') { if (test?._cypressTestStatusInfo?.outerStatus === 'failed') { - // We call _runner.fail here instead of in mocha because we need to make sure none of the hooks mutate the current test state, which might trigger - // another attempt. This affects our server reporter by reporting the test final status multiple times and incorrect attempt statuses. - // We can be sure here that the test is settled and we can fail it appropriately if the condition is met. + // We call _runner.fail here instead of in mocha because we need to make sure none of the hooks mutate the current test state, which might trigger + // another attempt. This affects our server reporter by reporting the test final status multiple times and incorrect attempt statuses. + // We can be sure here that the test is settled and we can fail it appropriately if the condition is met. // In this case, since the last attempt of the test does not contain an error, we need to look one up from a previous attempt // and fail the last attempt with this error to appropriate the correct runner lifecycle hooks. However, we still want the // last attempt to be marked as 'passed'. This is where 'forceState' comes into play (see 'calculateTestStatus' in ./driver/src/cypress/mocha.ts) // If there are other hooks (such as multiple afterEach hooks) that MIGHT impact the end conditions of the test, we only want to fail this ONCE! - let lastTestWithErr = (test.prevAttempts || []).find((t) => t.state === 'failed') + const lastTestWithErr = (test.prevAttempts || []).find((t) => t.state === 'failed') // TODO: figure out serialization with this looked up error as it isn't printed to the console reporter properly. const err = lastTestWithErr?.err