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

fix(deps): regenerate lockfile to reduce dependencies #4083

Closed
wants to merge 1 commit into from
Closed

fix(deps): regenerate lockfile to reduce dependencies #4083

wants to merge 1 commit into from

Conversation

lukasholzer
Copy link
Collaborator

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes #<replace_with_issue_number>


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@lukasholzer lukasholzer added the type: chore work needed to keep the product and development running smoothly label Jan 19, 2022
@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 19, 2022

If you guys want, you can land #4072 and #4075 before this PR since they are included in this PR.

The installation error fix must come from the new @bugsnag packages included here.

erezrokah
erezrokah previously approved these changes Jan 19, 2022
@lukasholzer lukasholzer added automerge Add to Kodiak auto merge queue and removed automerge Add to Kodiak auto merge queue labels Jan 19, 2022
@XhmikosR
Copy link
Contributor

The Windows CI failure seems to happen on main sporadically too.

BTW, the E2E tests are pretty nice, I can see a few good warnings :)

@lukasholzer
Copy link
Collaborator Author

The Windows CI failure seems to happen on main sporadically too.

BTW, the E2E tests are pretty nice, I can see a few good warnings :)

I'm already curating a list with all flaky builds as I want to reduce them sadly our tests are not written that well so they use the file system and depend heavily on it and on API calls this makes it even more likely to be flaky.

But this topic has my full attention as it slows us really down.

@lukasholzer
Copy link
Collaborator Author

@erezrokah I'm retriggering now for the 4th time this PR and still not getting green always a different test that is flaky!

We need to take some action on this.

@erezrokah
Copy link
Contributor

@erezrokah I'm retriggering now for the 4th time this PR and still not getting green always a different test that is flaky!

We need to take some action on this.

I suggest to disable the flaky tests for now, as they are not helpful. We can work on them as a separate issue and enable when we stabilize them.

@XhmikosR
Copy link
Contributor

I say better trigger renovate to update build, config etc, and ping me to redo this PR one last time :)

@lukasholzer
Copy link
Collaborator Author

lukasholzer commented Jan 19, 2022

@XhmikosR could you give me some insights on how you cleaned it up? (I would love to understand the resolution of a shrinkwrap file more)

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 19, 2022

Just remove the lock file, git clean -dfx or rm -rf node_modules, npm install && npm shrinkwrap.

But in this particular case I've kept back 2 packages because they are deprecated (the new superagent and supertest versions).

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 19, 2022

It seems the size benchmark doesn't run here (probably because I'm the patch author) so here are the results:

main: 1357 packages: 234 MB (245.925.748 bytes) - 19.964 Files, 3.628 Folders
this: 1332 packages: 232 MB (243.856.025 bytes) - 19.728 Files, 3.586 Folders

I can't keep rebasing this forever so I'd say first land #4072 and #4090 and then this one. :)

@lukasholzer
Copy link
Collaborator Author

still flaky windows tests 😢

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 21, 2022

Have you guys tried to reduce ava concurrency? I know that the Actions Windows env is not exactly the same as vanilla Windows, but whenever I run the cli tests on my Windows VM with the default scripts, I was getting hundreds of the permissions errors. If I use --serial, it works better.

Generally, I'm not sure if spawning so many processes is good regardless of OS. You can see some tests take minute(s) to finish, not because they are slow, but probably because the system is being bombarded with many simultaneous tasks.

EDIT: OK, I give up :/ The EBUSY: resource busy or locked errors still happen with serial too. That being said, it's probably a good idea to stop using concurrency:5; the runners only have 2 cores IIRC. This is probably faster than before.
EDIT2: I reverted the test patches but kept them to compare the times/results. It shows that using the default concurrency is faster, at least on Ubuntu with the latest Node.js. It also seems it fixes some failures on Ubuntu with Node.js 12.x, but this might just be a fluke

@erezrokah
Copy link
Contributor

Ava defaults to concurrency 2 in CI:
https://github.com/avajs/ava/blob/6b63b1bc14a1aaa7d4ee4f2201f901624419c677/lib/api.js#L244

I believe this number only impacts the number of specs to run in parallel (a spec maps out to a file), and not number of tests in parallel within a single spec.

If that's the case, we might benefit from splitting specs with many tests to reduce the number of processes.

@lukasholzer
Copy link
Collaborator Author

Ava defaults to concurrency 2 in CI: https://github.com/avajs/ava/blob/6b63b1bc14a1aaa7d4ee4f2201f901624419c677/lib/api.js#L244

I believe this number only impacts the number of specs to run in parallel (a spec maps out to a file), and not number of tests in parallel within a single spec.

If that's the case, we might benefit from splitting specs with many tests to reduce the number of processes.

That's a good idea worth trying at least! 💡

@XhmikosR
Copy link
Contributor

Weird, the CI time did differ a lot...

Anyway, I hope you guys can improve this too later :)

@lukasholzer lukasholzer added the automerge Add to Kodiak auto merge queue label Jan 22, 2022
@kodiakhq kodiakhq bot removed the automerge Add to Kodiak auto merge queue label Jan 22, 2022
@kodiakhq
Copy link
Contributor

kodiakhq bot commented Jan 22, 2022

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

@XhmikosR
Copy link
Contributor

This is moot now since this was done in another commit. Similarly, #4072 is moot too.

@XhmikosR XhmikosR deleted the deps branch January 22, 2022 06:39
@lukasholzer
Copy link
Collaborator Author

@XhmikosR sorry can you please resolve the conflicts?

Seems like the disabled windows tests where solving the flakyness

@XhmikosR
Copy link
Contributor

I can't rebase because I dropped the branch since you included these changes in #3904

@lukasholzer
Copy link
Collaborator Author

Then I will close the PR. I try to open up the regeneration in in a new one :)

@XhmikosR
Copy link
Contributor

Shouldn't really help anymore :)

@lukasholzer
Copy link
Collaborator Author

yea I already did it on a PR that got merged so I hope we are fine now :)

Thx for your continues effort on this topic @XhmikosR 🙏 we are so glad you helped with your expertise! I'm saying this in behalf of the whole team @netlify/build-services :)

@XhmikosR
Copy link
Contributor

I've added most of my suggestions in #3941. It's a lot, I know, but I've already submitted quite a few upstream PRs.

Personally, I'd freeze adding new packages for a while and address some of those issues.

Unsure if this branch even works (due to the listr2 switch)n but the results are promising:

8.13.1: 1343 packages: 235 MB (246.929.792 bytes) - 20.563 Files, 3.622 Folders
branch: 1286 packages: 180 MB (189.142.725 bytes) - 18.077 Files, 3.194 Folders

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants