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

Fix lerna e2e publish #3173

Merged
merged 7 commits into from
Nov 7, 2019
Merged

Fix lerna e2e publish #3173

merged 7 commits into from
Nov 7, 2019

Conversation

cgewecke
Copy link
Collaborator

@cgewecke cgewecke commented Oct 31, 2019

Re-opens 3170 in a local context because there's some git logic that's necessary to trick Lerna into publishing which is not working when the changes are being proposed from a fork. It would be nice if this ran normally for outside contributors, but perhaps its not critical because this kind of test is more of a pre-publication test / sanity-check. It should work when an outside contribution is merged - that build can always be verified if the reviewer had questions...

Unfortunately #3157 was only ~%25 correct :/

PR fixes several problems:

  • The packages were not being published by the lerna command correctly - they used exec (obviously wrong). I've had to upgrade Lerna to 3.x to get this to work.
  • Unexpectedly, truffle/contract mostly uses a Web3 from a different truffle module than itself, even though Web3 is a direct dependency there. Both modules need to be modified to get the tests to consume the virtual publication correctly

I've added a 'proof' that this works - there's an incorrect console.log line in web3-core-helpers which is printed out while truffle's tests run. So we know their code is exercising the published copy.
It needs to be removed before merging..

Lastly, some of Truffle's tests are failing with an error that's already been reported at truffle 2512. I don't think any recent changes here are the cause because they have not updated to 1.2.2 yet.

Actually it looks like they have some tests failing as direct result of changes in 1.2.2. See review discussion below.

@coveralls
Copy link

coveralls commented Oct 31, 2019

Coverage Status

Coverage remained the same at 84.347% when pulling fd8f3a7 on tests/e2e-publish-fix-web3 into a5e832f on 1.x.

@cgewecke
Copy link
Collaborator Author

@nivida Apologies for the earlier PR, this is now working as expected for both PR and Push builds. The truffle tests fail for reasons given in the PR description above, but the publication / installation part of this finally seems correct:

@cgewecke cgewecke requested a review from nivida October 31, 2019 20:26
@nivida nivida added 1.x 1.0 related issues CI labels Nov 4, 2019
Copy link
Contributor

@nivida nivida left a comment

Choose a reason for hiding this comment

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

@cgewecke Thanks for referencing those builds but unfortunately both of them do fail and we didn't introduce a breaking change.

package.json Show resolved Hide resolved
@cgewecke
Copy link
Collaborator Author

cgewecke commented Nov 5, 2019

@nivida

Thanks for referencing those builds but unfortunately both of them do fail and we didn't introduce a breaking change.

In fact Truffle is not able to upgrade to 1.2.2 because of several problems including #3175. I have opened an issue over there notifying them.

There is also a failing test for confirmations over http which could be related to the changes in #3142. Those changes should get some additional E2E tests here to verify their behavior is what we're expecting.

So overall I think this E2E is working as expected now (but was broken before when I thought everything was ok).

@nivida
Copy link
Contributor

nivida commented Nov 6, 2019

In fact Truffle is not able to upgrade to 1.2.2 because of several problems including #3175. I have opened an issue over there notifying them.

Great! Thanks! I was in contact some days ago with Nicholas from truffle as well. 👌

There is also a failing test for confirmations over http which could be related to the changes in #3142. Those changes should get some additional E2E tests here to verify their behavior is what we're expecting.

Hmm, the behaviour didn't change. I will get in contact with the right people at truffle to do further investigations.

So overall I think this E2E is working as expected now (but was broken before when I thought everything was ok).

Yep, looks good! Thanks! 💪

Feel free to resolve the conflicts and merge this PR.

@cgewecke
Copy link
Collaborator Author

cgewecke commented Nov 6, 2019

@nivida I am not allowed to merge without 2 approving reviews (which is good! Safer.)

This is technically merge-able although it might need a quick patch in a few days as well because some things about how web3 is being installed are under review at Truffle atm (see here).

So merge or wait idk - am watching developments over there and will fix as necessary.

Also looks like http confirmations handler is ok at Truffle now with 1.2.2.

@nivida nivida merged commit 22df832 into 1.x Nov 7, 2019
@nivida nivida deleted the tests/e2e-publish-fix-web3 branch November 14, 2019 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants