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

Update/shrinkwrap #12356

Merged
merged 1 commit into from
Mar 22, 2017
Merged

Update/shrinkwrap #12356

merged 1 commit into from
Mar 22, 2017

Conversation

oandregal
Copy link
Contributor

While rebasing and shrinkwrapping an unrelated PR #6945 I found that two tests were failing. It seems that the cause is that npm-shrinkwrap.json is outdated in master, so I prefer separate concerns and merge the other PR after this is solved.

@oandregal oandregal self-assigned this Mar 21, 2017
@matticbot
Copy link
Contributor

@oandregal
Copy link
Contributor Author

Tested the reader and all seem to work as expected. cc @blowery for review, in case I missed something.

@oandregal oandregal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. labels Mar 21, 2017
@nb
Copy link
Member

nb commented Mar 21, 2017

This works well, though before we can merge it, could you help me understand why exactly did the tests break? Did they break after shrink-wrapping?

@oandregal
Copy link
Contributor Author

This is going to be fun.

Instructions to reproduce the bug:

git checkout master
make distclean
make test 
// tests pass

make shrinkwrap
make test 
// tests fail 

Unfortunately, these instructions would have let you to reproduce the bug yesterday in the morning, but not today. More specifically, this bug is only reproducible if you did the shrinkwrap at any point between 2017-03-20 18:10 and 2017-03-21 10:39.

Reason is: the source of error is an update in the fbjs library which was fixed a few hours ago. Doing a shrinkwrap now will pull the latest version with the fix in place.

A more detailed explanation:

@oandregal oandregal changed the title Update/shrinkwrap and fix tests Update/shrinkwrap Mar 22, 2017
@oandregal
Copy link
Contributor Author

I still prefer merging this as a separate PR, and then merge #6945

@nb could you review and green-lit this PR?

@oandregal
Copy link
Contributor Author

Ah, forgot to mention: the tests no longer need fixing for the reason stated above, a mere shrinkwrap will suffice. Rebased the PR with the latest master.

@nb nb self-requested a review March 22, 2017 10:56
Copy link
Member

@nb nb left a comment

Choose a reason for hiding this comment

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

👍

@oandregal oandregal merged commit 4349b5e into master Mar 22, 2017
@oandregal oandregal deleted the update/shrinkwrap branch March 22, 2017 11:00
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 22, 2017
@oandregal oandregal added Build Framework and removed [Feature] Reader The reader site on Calypso. labels Mar 22, 2017
@matticbot matticbot added the [Size] M Medium sized issue label Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants