fix: browser-skipped tests are correctly recorded to the dashboard #24217
Conversation
…/cypress into organize-server-files
|
Thanks for taking the time to open a PR!
|
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (fnType === 'Test') { | ||
| // persist the original callback so we can send it to the cloud | ||
| // to prevent it from being registered as a modified test | ||
| runnable.body = testCallback.toString() |
There was a problem hiding this comment.
Since testCallback is always defined - either a function or a string (usually an empty string) - it looks like we could remove the if, and just always runnable.body = testCallback.toString(), regardless of fnType. Then we'd always have runnable.body defined, and the shape would be more consistent.
Or even better, set '' as a default argument, and skip the || ''s below in overloadMochaFnForConfig.
There was a problem hiding this comment.
We only want to set on Test body for now.
packages/driver/src/cypress/mocha.ts
Outdated
| function invokeFnWithOriginalTitle (ctx, originalTitle, mochaArgs, fn, _testConfig) { | ||
| const ret = fn.apply(ctx, mochaArgs) | ||
| type MochaArgs = [string, Function | undefined] | ||
| function createRunnable (ctx, fnType: 'Test' | 'Suite', mochaArgs: MochaArgs, runnableFn: Function, testCallback: Function | string, _testConfig?: Record<string, any>) { |
There was a problem hiding this comment.
If we set a default value of _testConfig?: Record<string, any> = {}, then we'll always have one, and can remove the conditional around if (_testConfig) {} both here and later in this file.
In general I'm a fan of default values as a way of avoiding checking if a thing is defined - just make sure it always is, one way or another.
| return $utils.reduceProps(runnable, RUNNABLE_PROPS) | ||
| } | ||
|
|
||
| const wrapAll = (runnable): any => { | ||
| // Reduce runnable down to its props and collections. | ||
| // Sent the the Reporter to populate command log |
There was a problem hiding this comment.
Sent by who? I'm not really sure what this comment is trying to tell me.
Edit: I think it makes more sense below where wrapAll is used, rather than up here where it's defined.
…cypress into report-skipped-tests
| @@ -9,7 +9,7 @@ import $stackUtils, { StackAndCodeFrameIndex } from './stack_utils' | |||
| import $utils from './utils' | |||
| import type { HandlerType } from './runner' | |||
|
|
|||
| const ERROR_PROPS = 'message type name stack parsedStack fileName lineNumber columnNumber host uncaught actual expected showDiff isPending docsUrl codeFrame'.split(' ') | |||
| const ERROR_PROPS = ['message', 'type', 'name', 'stack', 'parsedStack', 'fileName', 'lineNumber', 'columnNumber', 'host', 'uncaught', 'actual', 'expected', 'showDiff', 'isPending', 'docsUrl', 'codeFrame'] as const | |||
Ensure the test body and original test title is always recorded to the cloud, regardless of browser-skip state, to ensure the cloud does not incorrectly mark the test as "new" or "modified".
Test Steps:
Verified by recording to Staging & running system-tests/projects/e2e/record_config.cy.js using Edge & Chrome:
Showing Issue:
Showing Fixes:
User facing changelog
Fixed as issue where browser-skipped tests were incorrectly recorded to the dashboardDashboard which resulted in the Dashboard marking the test as "new" or "modified" when it already existed. Fixes #23517.
How has the user experience changed?
Browser-skipped tests are correctly recorded to the dashboard.
PR Tasks
cypress-documentation?type definitions?