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.

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

test(system-tests): support npm for test projects #20664

Merged
merged 20 commits into from
Mar 22, 2022
Merged

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Mar 17, 2022

User facing changelog

n/a

Additional details

Actual diff minus lockfile changes is +400 -283

  • Previously, system-tests was updated to allow test projects to install dependencies with yarn: test: node_modules installs for system-tests, other improvements #18574
  • We want to expand system-tests to support testing arbitrary example projects. Many of our example projects use npm.
  • This updates the dep installation code to support npm as well if package-lock.json is detected.
  • The vite-ct project has been converted to using npm to prove that this works.
  • Also refactors the dep installation code to support this change - instead of being jammed into fixtures.ts, it has been moved to a dep-installer subfolder

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?
  • [na] Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 17, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Mar 17, 2022



Test summary

19339 0 218 0Flakiness 2


Run details

Project cypress
Status Passed
Commit 1347ba7
Started Mar 18, 2022 8:31 PM
Ended Mar 18, 2022 8:43 PM
Duration 11:50 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

reporter.hooks.spec.js Flakiness
1 hooks > can rerun without timeout error leaking into next run (due to run restart)
cypress/proxy-logging_spec.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second

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

@flotwig flotwig changed the base branch from develop to refactor-dep-installer March 17, 2022 21:46
@flotwig flotwig force-pushed the refactor-dep-installer branch from 2b1b07e to 360d8b7 Compare March 17, 2022 21:47
@flotwig flotwig changed the base branch from refactor-dep-installer to develop March 17, 2022 21:48
@flotwig flotwig marked this pull request as ready for review March 18, 2022 14:13
@flotwig flotwig requested a review from a team as a code owner March 18, 2022 14:13
@flotwig flotwig requested review from jennifer-shehane and removed request for a team and jennifer-shehane March 18, 2022 14:13
@@ -38,4 +38,6 @@
// Volar is the main extension that powers Vue's language features.
// "volar.autoCompleteRefs": false,
"volar.takeOverMode.enabled": true,

"editor.tabSize": 2,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not necessarily related to this PR, but I noticed sometimes vscode doesn't know what our tab size is (where does that even come from? eslint?) and will default to 4.

JessicaSachs
JessicaSachs previously approved these changes Mar 18, 2022
Copy link
Contributor

@JessicaSachs JessicaSachs left a comment

Choose a reason for hiding this comment

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

👍🏻 This looks good. It's difficult to manually test all of the edge cases you're covering, but the basics seem to be working for me.

}
}

export async function scaffoldCommonNodeModules () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment above this line describing what it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Seems fine! Left some comments but nothing blocking a merge.

optionalDependencies?: Dependencies
}

const log = (...args) => console.log('📦', ...args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking here, but a fun TS trick to get the correct type of ...args:

const log = (...args: Parameters<typeof console.log>) => console.log('📦', ...args)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks for the tip. I think I'll leave this one as-is since it's so simple and we use this pattern elsewhere but I'll keep Parameters in mind.

await symlinkNodeModulesFromCache(tmpNodeModulesDir, cacheNodeModulesDir)
} else {
// due to an issue in NPM, we cannot have `node_modules` be a symlink. fall back to copying.
// https://github.com/npm/npm/issues/10013
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the linked issue is 3-4 years old - I wonder if/when this will be fixed, or if it's a wontfix. 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure, I unfortunately wasn't able to find a corresponding issue in their un-archived repo. But it's definitely still an issue.

await fs.symlink(targetDir, destDir, 'junction')
}

// 8. If necessary, ensure that the `node_modules` cache is updated by copying `node_modules` back.
Copy link
Contributor

Choose a reason for hiding this comment

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

Big 👍 to the comments here

@flotwig flotwig merged commit f2100a8 into develop Mar 22, 2022
@flotwig flotwig deleted the system-tests-npm branch March 22, 2022 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants