Skip to content
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

fix(vite-dev-server): Retry dynamic importing of support file and spec file when they fail during CT run with Vite #26202

Closed
wants to merge 7 commits into from
Closed

fix(vite-dev-server): Retry dynamic importing of support file and spec file when they fail during CT run with Vite #26202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3abe635
test retrying dynamic module imports for Vite support file and spec f…
astone123 Mar 22, 2023
a478d7d
[run ci]
astone123 Mar 22, 2023
6cecebc
use console.error so we can see in the debug logs; add test
astone123 Mar 23, 2023
240a4c0
[run ci]
astone123 Mar 23, 2023
164ef2e
log on the first retry so that we know it happened [run ci]
astone123 Mar 23, 2023
ed984ef
try building binaries again [run ci]
astone123 Mar 23, 2023
243502a
add comment and changelog entry
astone123 Mar 24, 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
6 changes: 3 additions & 3 deletions .circleci/workflows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ mainBuildFilters: &mainBuildFilters
- /^release\/\d+\.\d+\.\d+$/
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- 'update-v8-snapshot-cache-on-develop'
- 'emily/before-spec-promise'
- 'retry-dynamic-imports'

# usually we don't build Mac app - it takes a long time
# but sometimes we want to really confirm we are doing the right thing
Expand All @@ -41,7 +41,7 @@ macWorkflowFilters: &darwin-workflow-filters
- equal: [ develop, << pipeline.git.branch >> ]
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
- equal: [ 'emily/before-spec-promise', << pipeline.git.branch >> ]
- equal: [ 'retry-dynamic-imports', << pipeline.git.branch >> ]
- matches:
pattern: /^release\/\d+\.\d+\.\d+$/
value: << pipeline.git.branch >>
Expand Down Expand Up @@ -139,7 +139,7 @@ commands:
- run:
name: Check current branch to persist artifacts
command: |
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "emily/before-spec-promise" && "$CIRCLE_BRANCH" != "update-v8-snapshot-cache-on-develop" ]]; then
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "retry-dynamic-imports" && "$CIRCLE_BRANCH" != "update-v8-snapshot-cache-on-develop" ]]; then
echo "Not uploading artifacts or posting install comment for this branch."
circleci-agent step halt
fi
Expand Down
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ _Released 03/28/2023 (PENDING)_

- Fixed a compatibility issue so that component test projects can use [Vite](https://vitejs.dev/) version 4.2.0 and greater. Fixes [#26138](https://github.com/cypress-io/cypress/issues/26138).
- Changed the way that Git hashes are loaded so that non-relevant runs are excluded from the Debug page. Fixes [#26058](https://github.com/cypress-io/cypress/issues/26058).
- Fixed an issue where Vite component tests sometimes fail in CI due to not being able to dynamically import the component support or spec modules. Fixes [#25913](https://github.com/cypress-io/cypress/issues/25913).

**Misc:**

Expand Down
40 changes: 38 additions & 2 deletions npm/vite-dev-server/client/initCypressTests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,42 @@
// This file is merged in a <script type=module> into index.html
// it will be used to load and kick start the selected spec

// Fetch a dynamic import and re-try 3 times with a 2-second back-off.
// We want to re-try these if they fail because sometimes (seemingly due to network issues)
// the modules aren't available when we first request them.
// See https://github.com/cypress-io/cypress/issues/25913
async function importWithRetry (importFn) {
try {
return await importFn()
} catch (error) {
for (let i = 0; i < 3; i++) {
await new Promise((resolve) => setTimeout(resolve, 1000 * 2 ** i))

let url

try {
// Get request URL from error message from original import
url = new URL(
error.message
.replace('Failed to fetch dynamically imported module: ', '')
.trim(),
)
Comment on lines +19 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, what sort of details do we get from this error message? Can we tell the difference between a module not being ready and the dev server being totally hosed (misconfigured, etc) so we don't unnecessarily hang the test on retries if it can't possibly succeed?


console.error(`retrying import of ${url?.href}`)

// add a timestamp to the end of the URL to force re-load the module instead of using the cache
url.searchParams.set('t', `${+new Date()}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
url.searchParams.set('t', `${+new Date()}`)
url.searchParams.set('t', `${Date.now()}`)


return await import(url.href)
} catch (e) {
console.error(`retrying import of ${url?.href}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick - will print a message that we're going to retry on the last attempt

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, maybe more of a warning than an error if we're potentially going to recover from it with a retry. A total failure constitutes an error, at least in my mind

}
}

throw error
}
}

const CypressInstance = window.Cypress = parent.Cypress

const importsToLoad = []
Expand All @@ -25,7 +61,7 @@ if (supportFile) {
// We need a slash before /cypress/supportFile.js, this happens by default
// with the current string replacement logic.
importsToLoad.push({
load: () => import(`${devServerPublicPathRoute}${supportRelativeToProjectRoot}`),
load: () => importWithRetry(() => import(`${devServerPublicPathRoute}${supportRelativeToProjectRoot}`)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we refactor importWithRetry to accept the string path we want to import and just do the import(..) in there and repeat as needed? Then we wouldn't have to parse out the URL from the error message in that function

absolute: supportFile,
relative: supportRelativeToProjectRoot,
relativeUrl: `${devServerPublicPathRoute}${supportRelativeToProjectRoot}`,
Expand All @@ -41,7 +77,7 @@ const testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs/${normalizedA
/* Spec file import logic */
// We need a slash before /src/my-spec.js, this does not happen by default.
importsToLoad.push({
load: () => import(testFileAbsolutePathRoute),
load: () => importWithRetry(() => import(testFileAbsolutePathRoute)),
absolute: CypressInstance.spec.absolute,
relative: CypressInstance.spec.relative,
relativeUrl: testFileAbsolutePathRoute,
Expand Down
15 changes: 15 additions & 0 deletions npm/vite-dev-server/cypress/e2e/vite-dev-server.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,21 @@ describe('Config options', () => {
expect(verifyFile).to.eq('OK')
})
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Great to see a test. Didn't we decide this would be a run-mode only thing, though (this test is for open mode - you'd need to write it in system-tests for a run mode test)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right - while I was struggling to write a system test I started to question why this should be run only. Maybe the reason that it should be is that we don't want to modify the dynamic import behavior in open mode? We could make it run only and I'll have to come up with a testing strategy that doesn't use cy.intercept

it('retries importing support file if the import errors', () => {
cy.intercept('**/cypress/support/component.js', {
statusCode: 404,
})

cy.scaffoldProject('vite2.9.1-react')
cy.openProject('vite2.9.1-react', ['--config-file', 'cypress-vite.config.ts'])
cy.startAppServer('component')

cy.visitApp()
cy.contains('App.cy.jsx').click()
cy.waitForSpecToFinish()
cy.get('.passed > .num').should('contain', 2)
})
})

describe('sourcemaps', () => {
Expand Down