Skip to content

fix(core): silent failures when linking forge dependencies in tests - #3219

Merged
erickzhao merged 8 commits into
electron:mainfrom
ronaldcurtis:fix-package-linking
Apr 26, 2023
Merged

fix(core): silent failures when linking forge dependencies in tests#3219
erickzhao merged 8 commits into
electron:mainfrom
ronaldcurtis:fix-package-linking

Conversation

@ronaldcurtis

@ronaldcurtis ronaldcurtis commented Apr 23, 2023

Copy link
Copy Markdown
Contributor
  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

When I was trying to debug another problem in a PR by @caoxiemeihao (#3178), I noticed we were getting silent errors when it came to linking forge dependencies in tests involving api.init().

This PR fixes the linking of local forge dependencies in our tests that use api.init.

How does this PR help?

  • It helps prevent faulty test results that involve api.init. For example, if changes are made across multiple forge packages, and if package linking silently fails in those related tests, then those tests are not properly testing changes that were made, and they're instead testing already-published packages, which makes them incorrect.
  • It helps debug our tests. When test failures occur and you're trying to figure out which packages could be affecting it, then properly linking packages helps us debug the impact of those packages involved.

@ronaldcurtis
ronaldcurtis requested a review from a team as a code owner April 23, 2023 22:34
Comment thread package.json Outdated
Comment thread package.json Outdated
"lint:fix": "prettier --write .",
"link:prepare": "lerna exec -- node ../../../tools/silent.js yarn link --link-folder ../../../.links --silent --no-bin-links",
"link:prepare": "lerna exec -- yarn link --silent --no-bin-links --link-folder ../../../.links",
"link:remove": "lerna exec -- yarn unlink --silent --no-bin-links --link-folder ../../../.links",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added this script to help remove links in after mocha hooks.

for (const packageName of Object.keys(packageJson.devDependencies)) {
if (packageName.startsWith('@electron-forge/')) {
if (task) task.output = `${packageManager} link --link-folder ${linkFolder} ${packageName}`;
await yarnOrNpmSpawn(['link', '--link-folder', linkFolder, packageName], {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is where it was silently failing in our tests (found after wrapping it in a try catch):

Screen Shot 2023-04-23 at 4 11 42 pm

task: async (_, task) => {
await initLink(dir, task);
},
exitOnError: true,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Separated the linking task, and made it exitOnError: true so it no longer silently fails in tests. It will only run when LINK_FORGE_DEPENDENCIES_ON_INIT is truthy.

@BlackHole1
BlackHole1 requested a review from a team April 24, 2023 00:10

@BlackHole1 BlackHole1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread packages/api/core/src/api/init.ts

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for this!

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small note: it seems like the new yarn link:prepare command yields a lot of warnings if you run it twice in a row. Looks like running yarn link:remove between runs fixes that.

Not sure if that's expected behaviour but it used to just pass if you ran it consecutively.

@erickzhao
erickzhao self-requested a review April 25, 2023 23:38
@ronaldcurtis

Copy link
Copy Markdown
Contributor Author

Small note: it seems like the new yarn link:prepare command yields a lot of warnings if you run it twice in a row. Looks like running yarn link:remove between runs fixes that.

Not sure if that's expected behaviour but it used to just pass if you ran it consecutively.

@erickzhao Thanks for catching that 🙏 ^_^ I fixed and reintroduced the silent.js helper, so those warnings should be silenced now 🎉

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.

3 participants