chore: speed up serving-functions test by pre-generating oclif manifest #3517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oclif spends a lot of time scanning the functions directory. this adds up if we do it for every test, so we pre-generate it for testing.
netlify-team-account-1
added
the
type: chore
netlify-team-account-1
requested review from
minivan,
eduardoboucas and
ehmicky
📊 Benchmark resultsComparing with 8b95a5c Package size: 357 MB(no change) Legend
|
| @@ -65,7 +66,8 @@ | |||
| "test:ci:ava": "nyc -r json ava", | |||
| "docs": "node ./site/scripts/docs.js", | |||
| "watch": "nyc --reporter=lcov ava --watch", | |||
| "prepack": "oclif-dev manifest && npm prune --prod", | |||
| "build:manifest": "oclif-dev manifest", | |||
| "prepack": "npm run build:manifest && npm prune --prod", | |||
| "postpack": "rm -f oclif.manifest.json && npm i", | |||
There was a problem hiding this comment.
I see we currently try to only generate the manifest during the packing process. What's the idea behind that? Do we fear it becoming stale during development?
There was a problem hiding this comment.
Do we fear it becoming stale during development?
I'm not sure this was a very conscious choice, but yes - this can happen.
We could add another cleanup step to remove it (we'll need to propagate the exit code from the test run though).
|
(this PR is similar to #3508, but for speeding up local testing instead of CI) |
erezrokah
approved these changes
Oct 18, 2021
There was a problem hiding this comment.
Thanks @netlify-team-account-1, added a comment - I don't think it's a blocker.
Also, I think we can remove the change done in #3508, as this PR includes it
This reverts commit 0eea04d.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- Summary
oclif promises to only require the files that are neccessary for the command that is run. This is true if there's a
oclif.manifest.jsonfound - if it doesn't find it, it will require every single command synchronously. Sinceserving-functions.test.jsrunsntl devonce for every test (~25 at the moment), and there's no require caching since each test runs its own child process, this is a big time sink.By generating the
oclif.manifest.json, we can speed up these tests.- Test plan
tests shall still pass
- A picture of a cute animal (not mandatory but encouraged)