Skip to content

test: fix flaky migration test #25672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 5, 2023
Merged

test: fix flaky migration test #25672

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5b012b
test: fix flaky migration test [run ci]
ZachJW34 Feb 1, 2023
5d13579
Update packages/launchpad/cypress/e2e/migration.cy.ts
ZachJW34 Feb 3, 2023
d545ad7
Merge branch 'develop' into zachw/fix-flaky-migration-test
ZachJW34 Feb 3, 2023
5aa6c1a
Merge branch 'develop' into zachw/fix-flaky-migration-test
marktnoonan Feb 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions packages/data-context/src/actions/MigrationActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,7 @@ export class MigrationActions {
async initialize (config: LegacyCypressConfigJson) {
const legacyConfigForMigration = await this.setLegacyConfigForMigration(config)

// for testing mainly, we want to ensure the flags are reset each test
this.resetFlags()
this.reset(legacyConfigForMigration)

if (!this.ctx.currentProject || !legacyConfigForMigration) {
throw Error('cannot do migration without currentProject!')
Expand Down Expand Up @@ -435,11 +434,9 @@ export class MigrationActions {
}
}

resetFlags () {
reset (config?: LegacyCypressConfigJson) {
this.ctx.update((coreData) => {
const defaultFlags = makeCoreData().migration.flags

coreData.migration.flags = defaultFlags
coreData.migration = { ...makeCoreData().migration, legacyConfigForMigration: config }
})
}
}
1 change: 1 addition & 0 deletions packages/data-context/src/actions/ProjectActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export class ProjectActions {
d.app.browserStatus = 'closed'
})

this.ctx.actions.migration.reset()
await this.ctx.lifecycleManager.clearCurrentProject()
resetIssuedWarnings()
await this.api.closeActiveProject()
Expand Down
10 changes: 8 additions & 2 deletions packages/data-context/src/data/ProjectConfigManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,12 @@ export class ProjectConfigManager {
...loadConfigReply.requires,
this.configFilePath,
])

// 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()
Copy link
Contributor

@lmiller1990 lmiller1990 Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@ZachJW34 ZachJW34 Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.options.ctx.emitter.toApp()
}

return loadConfigReply.initialConfig
Expand All @@ -147,10 +153,10 @@ export class ProjectConfigManager {
this._state = 'errored'
await this.closeWatchers()

throw error
} finally {
this.options.ctx.emitter.toLaunchpad()
this.options.ctx.emitter.toApp()

throw error
}
}

Expand Down
12 changes: 7 additions & 5 deletions packages/data-context/src/data/ProjectLifecycleManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor

@lmiller1990 lmiller1990 Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@lmiller1990 lmiller1990 Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is this just a general QOL improvement since it's purely optimizations?

Copy link
Contributor Author

@ZachJW34 ZachJW34 Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@marktnoonan marktnoonan Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return null
}

const contents = await this.ctx.project.getConfig()

return contents.projectId ?? null
Expand Down Expand Up @@ -458,8 +463,6 @@ export class ProjectLifecycleManager {
const legacyConfigPath = path.join(projectRoot, this.ctx.migration.legacyConfigFile)

if (needsCypressJsonMigration && !this.ctx.isRunMode && this.ctx.fs.existsSync(legacyConfigPath)) {
this.legacyMigration(legacyConfigPath).catch(this.onLoadError)

return false
}

Expand All @@ -470,8 +473,9 @@ export class ProjectLifecycleManager {
return this.metaState.hasValidConfigFile
}

private async legacyMigration (legacyConfigPath: string) {
async legacyMigration () {
try {
const legacyConfigPath = path.join(this.projectRoot, this.ctx.migration.legacyConfigFile)
// we run the legacy plugins/index.js in a child process
// and mutate the config based on the return value for migration
// only used in open mode (cannot migrate via terminal)
Expand All @@ -480,8 +484,6 @@ export class ProjectLifecycleManager {
// should never throw, unless there existing pluginsFile errors out,
// in which case they are attempting to migrate an already broken project.
await this.ctx.actions.migration.initialize(legacyConfig)

this.ctx.emitter.toLaunchpad()
} catch (error) {
this.onLoadError(error)
}
Expand Down
4 changes: 4 additions & 0 deletions packages/frontend-shared/src/gql-components/HeaderBarContent.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,10 @@ mutation GlobalPageHeader_clearCurrentProject {
currentProject {
id
}
# This ensures the cache is updated with null after clearing project
migration {
configFileNameBefore
}
}
}
`
Expand Down
13 changes: 12 additions & 1 deletion packages/graphql/src/schemaTypes/objectTypes/gql-Query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,18 @@ export const Query = objectType({
t.field('migration', {
type: Migration,
description: 'Metadata about the migration, null if we aren\'t showing it',
resolve: (root, args, ctx) => ctx.coreData.migration.legacyConfigForMigration ? ctx.coreData.migration : null,
resolve: async (root, args, ctx) => {
// First check to see if "legacyConfigForMigration" is defined as that means we have started migration
if (ctx.coreData.migration.legacyConfigForMigration) return ctx.coreData.migration.legacyConfigForMigration

if (!ctx.migration.needsCypressJsonMigration()) {
return null
}

await ctx.lifecycleManager.legacyMigration()

return ctx.coreData.migration.legacyConfigForMigration
},
})

t.nonNull.field('dev', {
Expand Down
2 changes: 1 addition & 1 deletion packages/launchpad/cypress/e2e/migration.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ function renameSupport (lang: 'js' | 'ts' | 'coffee' = 'js') {
describe('global mode', () => {
// TODO: Figure out why it is flaky. Seems to be due to MigrationWizard query being executed multiple times
// see: https://github.com/cypress-io/cypress/issues/25377
it.skip('migrates 2 projects in global mode', () => {
it('migrates 2 projects in global mode', () => {
cy.openGlobalMode()
cy.addProject('migration-e2e-export-default')
cy.addProject('migration-e2e-custom-integration-with-projectId')
Expand Down