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

ci(docs-infra): store JS bundles as CI artifacts to debug size check flakes #37703

Closed
wants to merge 2 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Jun 24, 2020

As reported in #37699, the size of the main angular.io bundle sometimes ends up bigger than expected on CI. This usually goes away after rerunning the job a couple of times.

It is unclear what is causing this. In order to help debug the issue, this commit stores the JS files that are checked as part of the aio payload-size check as CI artifacts, where they can be retrieved from and inspected.

@gkalpak gkalpak added area: build & ci Related the build and CI infrastructure of the project comp: docs-infra state: WIP target: major This PR is targeted for the next major release labels Jun 24, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jun 24, 2020
@gkalpak gkalpak force-pushed the ci-aio-debug-size-failures branch 3 times, most recently from 8ddab71 to 8ba3a9f Compare June 24, 2020 20:02
…flakes

As reported in angular#37699, the size of the main angular.io bundle sometimes
ends up bigger than expected on CI. This usually goes away after
rerunning the job a couple of times.

It is unclear what is causing this. In order to help debug the issue,
this commit stores the JS files that are checked as part of the aio
payload-size check as CI artifacts, where they can be retrieved from and
inspected.
@gkalpak gkalpak force-pushed the ci-aio-debug-size-failures branch from 8ba3a9f to 75e2aad Compare June 24, 2020 20:26
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed state: WIP labels Jun 24, 2020
@gkalpak gkalpak marked this pull request as ready for review June 24, 2020 20:27
@@ -395,6 +399,22 @@ jobs:
- run: yarn --cwd aio test-a11y-score-localhost
# Check the bundle sizes.
- run: yarn --cwd aio payload-size
# When `payload-size` check fails, copy the files that where checked into `debugArtifactsPath`.
- run:
when: on_fail
Copy link
Member Author

Choose a reason for hiding this comment

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

You can see how this step works here: https://circleci.com/workflow-run/5837947b-493e-4310-a0ab-efeea4b1643c.
(For that run, I set when: on_success to have this step run even if previous steps succeeded.)

.circleci/config.yml Outdated Show resolved Hide resolved
const checkedFiles = fs.readdirSync(distDir).filter(x => /^(?:main|polyfills|runtime)-es2015\..+\.js$/.test(x));
fs.mkdirSync(artifactsDir);
checkedFiles.forEach(x => fs.copyFileSync(path.join(distDir, x), path.join(artifactsDir, x)));
"
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would prefer to put this code into a JS helper file and simply run it from here (passing the << parameters.debugArtifactsPath >> as an argument). This makes it easier to check the code for syntax errors, provides more space for adding explanatory comments, and also can be made resilient to changes in the cwd by using __dirname.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this appears to be a copy of the code further down, so moving it into its own file would reduce the duplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

I intended this to be a temporary, hacky way to gather some info for debugging. Therefore, I wanted to touch as few places as possible and make this as self-contained as possible.

If we want this implemented for real, we should indeed move it to a script, move the steps to a re-usable command in config.yml and make sure it only happens on payload-size check failures (currently it will happen for any failure).

Copy link
Member

Choose a reason for hiding this comment

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

I think a compromise - somewhere in-between - would be acceptable. :-)
Just because it is temporary (for now!!) I don't think it is nice to make CircleCI even more complicated than it is already.

.circleci/config.yml Outdated Show resolved Hide resolved
@gkalpak gkalpak force-pushed the ci-aio-debug-size-failures branch from db45aff to 1191888 Compare June 25, 2020 21:28
@mary-poppins
Copy link

You can preview 1191888 at https://pr37703-1191888.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-debug-size-failures branch from 1191888 to a6aafce Compare June 25, 2020 22:30
@mary-poppins
Copy link

You can preview a6aafce at https://pr37703-a6aafce.ngbuilds.io/.

@gkalpak gkalpak deleted the ci-aio-debug-size-failures branch June 26, 2020 12:11
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 27, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…flakes (angular#37703)

As reported in angular#37699, the size of the main angular.io bundle sometimes
ends up bigger than expected on CI. This usually goes away after
rerunning the job a couple of times.

It is unclear what is causing this. In order to help debug the issue,
this commit stores the JS files that are checked as part of the aio
payload-size check as CI artifacts, where they can be retrieved from and
inspected.

PR Close angular#37703
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants