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

test(ivy): temp disable payload limit tests #21940

Closed
wants to merge 1 commit into from
Closed

Conversation

kara
Copy link
Contributor

@kara kara commented Jan 31, 2018

Update payload size for ivy

@kara kara added action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy target: major This PR is targeted for the next major release labels Jan 31, 2018
@mhevery
Copy link
Contributor

mhevery commented Jan 31, 2018

@kara we discussed this with @IgorMinar and decided to disable the size tracking for render3 for now. Tha plan is to do it on symbol level. This is creating too much noise. Could you disable it?

//cc @chuckjaz

@kara kara changed the title test(ivy): update payload limits test(ivy): temp disable payload limit tests Jan 31, 2018
@jasonaden jasonaden added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 31, 2018
@jasonaden jasonaden closed this in 3db75b3 Jan 31, 2018
@IgorMinar
Copy link
Contributor

wait - this is not what we agreed on though. because you disabled it here, we no longer see the trend (see how the graph ends abruptly https://size.angular.io/hello_world__render3__rollup/master)

What we agreed on is that we will no longer break the build when the size changes. This was supposed to be done by removing these lines of code:

"hello_world__render3__closure": {
"master": {
"uncompressed": {
"bundle": 8153
}
}
},
"hello_world__render3__rollup": {
"master": {
"uncompressed": {
"bundle": 10034
}
}
},
"hello_world__render3__cli": {
"master": {
"uncompressed": {
"inline": 1447,
"main": 40513,
"polyfills": 61085
}
}
}

Can we please roll back this change and update the _payload-limits.json as I suggested?

@IgorMinar IgorMinar reopened this Feb 7, 2018
@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: merge The PR is ready for merge by the caretaker labels Feb 7, 2018
@kara
Copy link
Contributor Author

kara commented Feb 7, 2018

Apologies! Clearly misunderstood your meaning.

@IgorMinar
Copy link
Contributor

IgorMinar commented Feb 7, 2018 via email

@kara kara force-pushed the size branch 2 times, most recently from 8682348 to 6ceefc6 Compare February 7, 2018 22:11
@mhevery
Copy link
Contributor

mhevery commented Feb 8, 2018

Given that we have a new way of tracking the payload, is this still needed?

@IgorMinar
Copy link
Contributor

IgorMinar commented Feb 8, 2018 via email

@mhevery
Copy link
Contributor

mhevery commented Feb 12, 2018

@IgorMinar now that we buildsize bot is your comment still relevant?

@IgorMinar
Copy link
Contributor

yes. please read #21940 (comment)

@ngbot
Copy link

ngbot bot commented Feb 13, 2018

Hi @kara! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment
@ngbot
Copy link

ngbot bot commented Feb 13, 2018

Hi @kara! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@buildsize
Copy link

buildsize bot commented Apr 5, 2018

File name Previous Size New Size Change
bundle.min.js 52.79 KB 52.21 KB -586 bytes (1%)
bundle.min.js.brotli 14.16 KB 14.06 KB -101 bytes (1%)

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Apr 5, 2018
@IgorMinar IgorMinar closed this in 629629d Apr 5, 2018
@kara kara deleted the size branch October 13, 2018 01:09
@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 Sep 14, 2019
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 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

5 participants