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

Dropping ethereumjs-testing dependency #953

Merged
merged 17 commits into from
Nov 16, 2020
Merged

Conversation

evertonfraga
Copy link
Contributor

@evertonfraga evertonfraga commented Nov 13, 2020

Dropping the git submodule within a dependency is fundamental to the Yarn workspaces PR #900.

  • Add git submodule pointing to ethereum/tests
  • CI: Add git submodule on checkout instructions
  • Refactor tests VM
  • Refactor tests Tx
  • Remove all references to ethereumjs-testing on the code
  • Remove all references to ethereumjs-testing on the docs
  • Add error handling with message to load the git submodules
  • Documentation for contributors

@codecov
Copy link

codecov bot commented Nov 13, 2020

Codecov Report

Merging #953 (1625831) into master (222283b) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 75.04% <ø> (ø)
blockchain 77.39% <ø> (ø)
common 92.11% <ø> (+0.24%) ⬆️
ethash 82.08% <ø> (ø)
tx 86.04% <ø> (-0.22%) ⬇️
vm 87.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@evertonfraga evertonfraga force-pushed the ethereum-tests-submodule branch from e4bfba1 to 6da8ec4 Compare November 13, 2020 22:29
@evertonfraga evertonfraga self-assigned this Nov 13, 2020
@evertonfraga evertonfraga marked this pull request as ready for review November 13, 2020 22:34
@evertonfraga evertonfraga requested review from ryanio, holgerd77 and jochem-brouwer and removed request for ryanio and holgerd77 November 13, 2020 22:36
@evertonfraga evertonfraga force-pushed the ethereum-tests-submodule branch from 3b4dbf5 to 3e5ddb0 Compare November 14, 2020 07:04
@evertonfraga evertonfraga force-pushed the ethereum-tests-submodule branch from 3e5ddb0 to 7944442 Compare November 14, 2020 07:06
@ryanio
Copy link
Contributor

ryanio commented Nov 14, 2020

would it be possible to run the needed git submodule init; git submodule update commands as an install hook?

@evertonfraga
Copy link
Contributor Author

@ryanio it's something to think about: if we want to make the installation process to download more than 1GB worth of test files.

Given it's only used in VM and Tx tests, I believe the best would be to have an npm script, like pretest that checks the submodule out before each of those libraries test runs :)

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

This looks good, great, thanks Everton! 😄

(man, I already miss you)

@holgerd77 holgerd77 merged commit ce0cc55 into master Nov 16, 2020
@holgerd77 holgerd77 deleted the ethereum-tests-submodule branch November 16, 2020 09:12
@holgerd77
Copy link
Member

I just set this up locally - everything works - but I was getting a bit nervous since I realized post-merge that we are moving away - at least regarding the state of this PR - from versioning the submodule state by doing ethereumjs-testing releases on a concrete submodule commit state and therefore working on a stable state of the ethereum/tests repo.

I am actually totally puzzled that the tests here are passing beside this update, the ethereum/tests state we had freezed in the ethereumjs-testing v1.3.3 release and the current develop state are several months apart and on such ethereum/tests updates we always had something breaking in the VM in the past, so this would be a total primer if everything "just passes". 😄 Or - to not exclude any possibility - might this be some caching thing and in reality the old submodule state was used? 🤔 (just to think in all directions).

Anyhow: I am a bit unsure about this. In the past just going along with develop led to an unstable development and CI situation. On the other hand if we change the process here and just go along we can also provide much quicker feedback to the ethereum/tests guys, and are also always up with the latest tests, both might be beneficial for us internally as well as the overall test development situation.

What do others think? Is this worth a try? Or should we remain conservative here and reintroduce some static snapshot referencing (@evertonfraga would this still be possible within the current directly integrated submodule structure?)?

@holgerd77
Copy link
Member

Also: would this blow some CI stuff if we would have the folder in packages? Otherwise I would find this significantly more beautiful.

@evertonfraga
Copy link
Contributor Author

evertonfraga commented Nov 16, 2020

Hey!

The git submodule documentation and usage are a bit misleading, but submodules are always locked to a commit reference. I actually don't understand why the branch name is ever present on the .gitmodules file.

image

To lock it to the same commit ethereumjs-testing is using, please do:

cd ethereum-tests
git checkout 66a55cd42f63845e34767504d0a7a62b452a7e7a
cd -
git add ethereum-tests

Now, as for moving the submodule, newer versions of git support the same syntax as regular files:

git mv ethereum-tests packages/ethereum-tests

The following step would be to change file references to the ./packages directory, which should be straightforward (tests/testLoader.ts in both tx and vm).

The CI should work out of the box.

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

Successfully merging this pull request may close these issues.

3 participants