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

Jump to bottom

Fix for reporting correct bundle size #11544

Merged
merged 10 commits into from Aug 15, 2022
Merged

Fix for reporting correct bundle size #11544

merged 10 commits into from Aug 15, 2022

Conversation

sonalivdeshpande
Copy link
Contributor

@sonalivdeshpande sonalivdeshpande commented Aug 12, 2022 •
edited

The bundle size comparison tool is currently comparing baseline against main branch. The baseline for pull requests opened against the next branch are incorrect. This pull request aims to fix the baseline comparison by setting an env variable TARGET_BRANCH_NAME as branch that is the target of a pull request. By default, the env variable is set to main.

AB#1570

@sonalivdeshpande sonalivdeshpande requested a review from a team as a code owner August 12, 2022 23:50
@github-actions github-actions bot added the area: build Build related issues label Aug 12, 2022
@github-actions github-actions bot added the base: main PRs targeted against main branch label Aug 12, 2022
@sonalivdeshpande sonalivdeshpande changed the title Fix for reporting bundle size against next branch Fix for reporting correct bundle size against next branch Aug 12, 2022
@sonalivdeshpande sonalivdeshpande changed the title Fix for reporting correct bundle size against next branch Fix for reporting correct bundle size Aug 12, 2022
ChumpChief
ChumpChief reviewed Aug 13, 2022
View reviewed changes
Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

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

I think it looks reasonable - will be interested to see it run against this PR :)

tools/pipelines/templates/build-npm-package.yml Outdated Show resolved Hide resolved
@sonalivdeshpande
Copy link
Contributor Author

I think it looks reasonable - will be interested to see it run against this PR :)

bundle size not reported :(

@ChumpChief
Copy link
Contributor

I think it looks reasonable - will be interested to see it run against this PR :)

bundle size not reported :(

Might be due to that other bug we were observing, that it only successfully reports bundle size if the first run succeeds. Maybe try opening a fresh PR with this change and see if you can get a clean run?

@curtisman
Copy link
Member

Can we write some description for the PR? Not all people have access to azure work item

@curtisman
Copy link
Member

I think it looks reasonable - will be interested to see it run against this PR :)

bundle size not reported :(

Might be due to that other bug we were observing, that it only successfully reports bundle size if the first run succeeds. Maybe try opening a fresh PR with this change and see if you can get a clean run?

I think there is some heuristic to not report to GH if there is no difference

@sonalivdeshpande
Copy link
Contributor Author

sonalivdeshpande commented Aug 15, 2022 •
edited

For this PR, there was no difference in bundle size -- No size changes detected, skipping posting PR comment.

For one of the PR against which the bundle size was not reported: #11491 -- the logs are as follows:

2022-08-10T19:01:56.0187770Z The baseline commit for this PR is b2c560ab996ad4d3f5e4c84e35320502045e2aec
2022-08-10T19:01:56.0188737Z Trying backup baseline commit e87851120f912340ee8e63987560c3b78f89f52b
2022-08-10T19:01:56.0189767Z Trying backup baseline commit ad7b27a50d04b53ac1b92db95f1a6e1eff9fbe3e
2022-08-10T19:01:56.0190326Z Found baseline build with id: 84555
No size changes detected, skipping posting PR comment

Backup baseline commit is looked up when baseline build is undefined or successful baseline build does not have the needed build artifacts. Not sure why the baseline build is undefined in this case.

ChumpChief
ChumpChief approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

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

One nit on a comment, but looks good!

tools/bundle-size-tools/src/utilities/gitCommands.ts Outdated Show resolved Hide resolved
@sonalivdeshpande sonalivdeshpande merged commit 0026561 into microsoft:main Aug 15, 2022
@sonalivdeshpande sonalivdeshpande deleted the bundle-size-bug branch August 15, 2022 23:23
@github-actions
Copy link
Contributor

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

sonalivdeshpande added a commit that referenced this pull request Aug 25, 2022
@sonalivdeshpande
Fix for reporting correct bundle size (#11544)
29a4971
@alexvy86 alexvy86 mentioned this pull request Oct 26, 2022
Merged
@sonalivdeshpande sonalivdeshpande mentioned this pull request Nov 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChumpChief ChumpChief ChumpChief approved these changes

Assignees
No one assigned
Labels
area: build Build related issues base: main PRs targeted against main branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sonalivdeshpande @ChumpChief @curtisman