test: fix flaky migration test #25672
Conversation
Passing run #43800 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|||||||||||||||
emilyrohrbough
changed the title
| @@ -103,6 +103,11 @@ export class ProjectLifecycleManager { | |||
|
|
|||
| async getProjectId (): Promise<string | null> { | |||
| try { | |||
| // No need to kick off config initialization if we need to migrate | |||
| if (this.ctx.migration.needsCypressJsonMigration()) { | |||
There was a problem hiding this comment.
General question - if the test is only flaky, I wouldn't expect we need to touch production code to fix it. WDYT? Is there any additional risk here? Is the test hitting a scenario that's impossible in prod?
There was a problem hiding this comment.
Or is this just a general QOL improvement since it's purely optimizations?
There was a problem hiding this comment.
I think this test was particularly flaky because it migrated two projects, and the number of queries re-executed was doubled/tripled. Hard to reproduce locally but in slow CI environments I think a call might have been dropped in between states and caused the test to fail.
I don't think there is a fix for this outside touching production code. The number of requeries is a sign we aren't doing something correct so most of the changes was to reduce this behavior.
There was a problem hiding this comment.
if the test is only flaky, I wouldn't expect we need to touch production code to fix it. WDYT?
I see this a bit differently, a flaky test is often telling us about non-determinism in the app code that we don't see locally due to different system resources, OS, etc.
| // Only call "to{App,Launchpad}" once the config is done loading. | ||
| // Calling this in a "finally" would trigger this emission for every | ||
| // call to get the config (which we do a lot) | ||
| this.options.ctx.emitter.toLaunchpad() |
There was a problem hiding this comment.
Nice, I noticed we have a lot of excess gql calls in general, hope we can find a good way to monitor/track these eventually... no idea how other apps handle this, or do they just have lots of requests 🤔
Nice thing about REST is you are very deliberate about requests - just thinking out loud here, gql sure is nice but man it has some trade-offs (complexity, hard to really know what/when a call is triggered).
There was a problem hiding this comment.
I think it all boils down the the toLaunchpad calls. This causes every active query to retrigger. If properly designed, we could probably get away with removing this altogether. Or we could rely more heavily on subscriptions to update data.
| @@ -103,6 +103,11 @@ export class ProjectLifecycleManager { | |||
|
|
|||
| async getProjectId (): Promise<string | null> { | |||
| try { | |||
| // No need to kick off config initialization if we need to migrate | |||
| if (this.ctx.migration.needsCypressJsonMigration()) { | |||
There was a problem hiding this comment.
if the test is only flaky, I wouldn't expect we need to touch production code to fix it. WDYT?
I see this a bit differently, a flaky test is often telling us about non-determinism in the app code that we don't see locally due to different system resources, OS, etc.
* develop: (28 commits) chore: update changelog validation example (#25742) fix: Improve error handling around calls to `this.next` in middleware (#25702) chore: debug page tooltip distance and artifact border (#25727) fix: update newProject ref when switching between organizations in SelectCloudProjectModal (#25730) misc: Add max widths to debug page message states (#25725) chore: export types (#25714) chore: release @cypress/webpack-preprocessor-v5.16.3 chore: release @cypress/vue-v5.0.4 chore: release @cypress/grep-v3.1.4 chore: Fix flaky test (#25726) dependency(deps): update dependency debug to ^4.3.4 🌟 (#25699) feat: openInIDE for failed debug spec (#25691) test: fix flaky CT test by relying on query (#25706) test: fix flaky migration test (#25672) misc: style change for responsiveness (#25687) misc: set min widths for icons (#25684) chore(deps): update dependency markdown-it to v11.0.1 🌟 (#25698) chore: Fix flaky origin .wait() test (#25693) chore: unskip tests (#25676) chore: release @cypress/webpack-preprocessor-v5.16.2 ...
Closes #25377
Additional details
Changes how the Migration flow is kicked off. Before, it was invoked when a project was clicked which lead to an initial query loading with
nullvalues. Now, the query is what kicks off the migration.Most of the optimizations are around reducing the number of
toLaunchpadcalls the app was making, causing the query to re-execute multiple times.It was hard to reproduce this failure locally as it seems to be influenced by machine resources.
Steps to test
Run the
migrations.cy.tse2e test in LaunchpadHow has the user experience changed?
Screenshots show a reduced number of the migration query
PR Tasks
cypress-documentation?type definitions?