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

Speedup e2e test on github #14627

Merged
merged 5 commits into from Jun 3, 2022
Merged

Conversation

liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented Jun 1, 2022

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Speed up all e2e tests on github.
The jest is the most obvious, going from 16-18 minutes to 5-7 minutes.
The total time went from 20-25 minutes to 12 minutes.

  1. Enable yarn caching for all tests
  2. Enable skipLibCheck in jest.

We can produce less CO2!😄

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 1, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52137/

@liuxingbaoyu
Copy link
Contributor Author

liuxingbaoyu commented Jun 1, 2022

We can only see the effect when we merge this PR or re run it.

@liuxingbaoyu liuxingbaoyu marked this pull request as draft Jun 1, 2022
@liuxingbaoyu liuxingbaoyu marked this pull request as ready for review Jun 1, 2022
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 1, 2022

Thanks! CI run with these updates: https://github.com/liuxingbaoyu/babel/actions/runs/2419282915

.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
key: ${{ runner.os }}-yarn3-e2e-${{ matrix.project }}-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn3-e2e-${{ matrix.project }}-
- name: Clean babel cache
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jun 1, 2022

Choose a reason for hiding this comment

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

Q: why do we need to clean the cache?

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu Jun 1, 2022

Choose a reason for hiding this comment

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

Because I found that the latest compiled babel will not be used, but the cache will be used. The babel package changes between each different CI test.

@@ -58,6 +58,8 @@ if [ "$BABEL_8_BREAKING" = true ] ; then
"
fi

sed -i 's/"skipLibCheck": false,/"skipLibCheck": true,/g' tsconfig.json # Speedup
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jun 1, 2022

Choose a reason for hiding this comment

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

When we'll start publishing our type definitions we will need to re-enable this. However, they can safely be skipped for now.

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu Jun 1, 2022

Choose a reason for hiding this comment

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

Yes, the process was too slow and took about 8 minutes.

- name: Checkout code
uses: actions/checkout@v3
- name: Use Node.js latest
uses: actions/setup-node@v3
with:
node-version: "*"
- name: Get yarn3 cache directory path
Copy link
Contributor

@JLHwung JLHwung Jun 1, 2022

Choose a reason for hiding this comment

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

Q: Can we use cache: yarn in setup-node@v3?

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu Jun 1, 2022

Choose a reason for hiding this comment

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

It used to be like this, but that would use the same cache as ci, with only babel's dependencies, not what the E2E project needs.

Copy link
Contributor

@JLHwung JLHwung Jun 1, 2022

Choose a reason for hiding this comment

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

the best way is to use the hash of the target project's lock file as the key

Maybe we can try cache-dependency-path?

https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#caching-packages-data

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu Jun 1, 2022

Choose a reason for hiding this comment

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

#14627 (comment)
As mentioned here, that's certainly good, but requires us to split the test script file and put the cache action after pulling the third-party repository.

@liuxingbaoyu
Copy link
Contributor Author

liuxingbaoyu commented Jun 1, 2022

In fact, the best way is to use the hash of the target project's lock file as the key, but that requires a lot of changes, so I chose this, and it works well in most cases.

@liuxingbaoyu
Copy link
Contributor Author

liuxingbaoyu commented Jun 1, 2022

🥳🥳🥳🥳🥳
Now e2e tests are faster than CI tests!

@liuxingbaoyu
Copy link
Contributor Author

liuxingbaoyu commented Jun 1, 2022

https://github.com/babel/babel/runs/6695976283?check_suite_focus=true#step:10:987

In addition, this exception appeared again.
You can check out these two PRs.

#14575
#14572

@existentialism existentialism added the PR: Internal 🏠 A type of pull request used for our changelog categories label Jun 3, 2022
@JLHwung JLHwung merged commit 5b7d93e into babel:main Jun 3, 2022
34 checks passed
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 3, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants