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

Master values is always empty - doesn't compare to master size #155

Closed
erichdev opened this issue Oct 6, 2017 · 14 comments
Closed

Master values is always empty - doesn't compare to master size #155

erichdev opened this issue Oct 6, 2017 · 14 comments
Labels

Comments

@erichdev
Copy link

erichdev commented Oct 6, 2017

Do you want to request a feature or report a bug?
Bug?

What is the current behavior?
When I submit a pull request to master, I want bundlesize to compare the PR size to master size and include the message 'smaller than master, good job'. But master values from api.get() always comes back empty. api.enabled is true. Could it be that maybe api.set() never gets called? If so, I'm unsure why - current branch is master and current event is push.

bundlesize successfully posts the Github status, but it does not include the comparison to master.

If the current behavior is a bug, please provide the steps to reproduce.
I have BUNDLESIZE_GITHUB_TOKEN saved in Travis environment, bundlesize settings in my package.json. I push a commit to a branch and make a PR to master.

What is the expected behavior?
bundlesize compares the size of PR to master.

If this is a feature request, what is motivation or use case for changing the behavior?

Please mention other relevant information.

  • node version 6.11.1
  • npm version 5.0.3
  • Operating system
  • bundlesize version 0.15.2
  • CI you are using Travis
@siddharthkp
Copy link
Owner

Hi, thanks for taking the time to write a detailed issue 🙂

bundlesize sets the master values on the first merge/push event on master.

  1. Has a PR been merged after adding bundlesize to the repo?
  2. Do have Travis enabled for push events on master?

We can definitely improve the messaging to help out with this, suggestions welcome!

@erichdev
Copy link
Author

erichdev commented Oct 9, 2017

Thanks for the response @siddharthkp !

I tried merging the PR into master and then opening a new PR. It still compares to the defined threshold only, but does not compare the PR size to master.

I do have Travis enabled for 'Build branch updates' and 'Build pull request updates' -- those are the two options I see closest to enabling for push events on master.

@siddharthkp
Copy link
Owner

Just to make sure I have this right, you only see the first half of this message:

screenshot

Can you run bundlesize with debug on and share the results here, thanks!

bundlesize --debug

@erichdev
Copy link
Author

erichdev commented Oct 9, 2017

Yes, I was only seeing '3.6kb < 4kb', but not '(0.2kb larger than master...)'. I've got it working now -- I had to push directly to master (rather than merge a PR into master) to get the master values to save. So does that mean the master values will only get updated if pushing directly to master?

@siddharthkp
Copy link
Owner

So does that mean the master values will only get updated if pushing directly to master?

No, merge commits should work as well 🤔

Does your .travis.yml have a check for branch?

@erichdev
Copy link
Author

erichdev commented Oct 9, 2017

My .travis.yml only runs the build steps and runs bundlesize, no checks for branch.

Hm, I'm looking at my Travis logs from this morning. Here are the bundlesize debug results, in order of build:

  1. Merged a PR to master, master values is empty, api.set() was called
  2. Created a new branch and opened a PR. master values is empty
  3. Pushed a commit directly to master. master values has a value and api.set() is called
  4. Pushed a commit to the branch created in step 2 above, master values has a value

Let me know if there's any specific info from the debug results that might be helpful. Thanks!

@siddharthkp
Copy link
Owner

I'm out of ideas here. In step 1, when api.set() is called, it should have set the master values.

I'm afraid, because of whatever is wrong, your next PR will also not updates the values 😢

What's the user/repo that you are talking about, I'll check my API logs for errors

@erichdev
Copy link
Author

erichdev commented Oct 9, 2017

erichdev/office-ui-fabric-react. I appreciate your help 👍

@siddharthkp
Copy link
Owner

Hey, as far as I can see..

These are the last 2 commits:

5ac156a (merge commit for PR#15) set the values that are visible in logs for d9f67de

Am I looking at the wrong commits? Is there a merge commit before 5ac156a that I missed?

@erichdev
Copy link
Author

erichdev commented Oct 9, 2017

Yes those are the last two commits to master. After merge commit 5ac156a. I created branch bundlesize3 and opened PR #16 b6a1e2e. Should that not compare the size to master? Why is master values empty in that build?

Edit: To clarify, master values came up empty on PR 16 when I first opened the PR. I then made a push directly to master (Empty commit d9f67de), and another commit to bundlesize3 (Testing 3bf0a07). In this last commit, master values is no longer empty

@siddharthkp
Copy link
Owner

siddharthkp commented Oct 9, 2017

Check this timeline out:

  1. The travis job for b6a1e2e from #16 ran at 2017-10-09T17:38:33Z (doesn't have the values)

  2. The travis job for 5ac156a from #15 ran at 2017-10-09T17:41:01Z (Sets the values)

  3. The travis job for 3bf0a07 from #16 ran at 2017-10-09T18:14:00Z (has the values)

I don't think the jobs ran in the same order you commited them 😄

@siddharthkp
Copy link
Owner

This is a rare but very interesting edge case, I have no idea what should we do here 😅

@erichdev
Copy link
Author

erichdev commented Oct 9, 2017

I thought I had waited to see the bundlesize command get run in the Travis log of the merge commit before I opened the new PR, but maybe not. I probably did get the timing wrong 🤦‍♂️

This edge case probably became obvious only because I didn't have any master values previously saved. If a merge is committed right after a new PR is submitted, I guess the user could always restart the Travis build if they want to ensure that bundlesize compares to the latest version of master.

I really appreciate all your help! This package is awesome and could provide great insight.

@siddharthkp
Copy link
Owner

siddharthkp commented Oct 10, 2017

If a merge is committed right after a new PR is submitted, I guess the user could always restart the Travis build if they want to ensure that bundlesize compares to the latest version of master.

Sadly, I don' think folks will realise that they ended up looking at an outdated comparison, especially in the case of multiple people working on a team 🤔

I don't think I have a fix off the top of my head. Do you mind opening a new issue about this race condition detailing what happened? We would like to circle back to this sometime in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants