Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

Add Istanbul tests #42

Merged
merged 2 commits into from
Nov 12, 2019
Merged

Add Istanbul tests #42

merged 2 commits into from
Nov 12, 2019

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Sep 23, 2019

Update tests submodule to 52d5247 (Sep 22, 2019) which includes ethereum/tests#639.

@holgerd77
Copy link
Member

@s1na Great 👍, thought about this yesterday as well and prepared a release on the Ethereum/tests repo ethereum/tests#640. This is close to be merged - now also containing the Blake2b F tests merged later this day. Let's please wait here with merging and then update directly to the v7.0.0-beta.1 release tag.

Some grain of salt: the test repo contains various structural updates / changes on the directory structure, see the "Test Format Changes" section in the release notes (the biggest change being the introduction of this new legacy test suite, but also the BlockchainTests being moved to subfolders). This will likely make necessary some preparatory updates here on the library. We especially need to make sure that all tests are still run and no tests are just skipped due to folder changes.

Cheers
Holger

@holgerd77
Copy link
Member

(if you want you can nevertheless directly start working/checking against the tests and opening a WIP PR on the VM side pointing to this PR here)

@s1na
Copy link
Contributor Author

s1na commented Oct 23, 2019

I updated the submodule to v7.0.0-beta.1.

Regarding the structural changes, what do you suggest we do? You mentioned that

This will likely make necessary some preparatory updates here on the library. We especially need to make sure that all tests are still run and no tests are just skipped due to folder changes.

Do you have some specific preparatory updated in mind? Alternatively we could update how the other libraries import the tests.

@holgerd77
Copy link
Member

@s1na Thanks for updating this, also wrote a short message on Gitter.

I would have a tendency to keep this completely transparent, so manage the changed folders internally here - so manage here some array of the legacy HFs and then internally fetch from the sub folder - and pass this on to the VM and keep the VM code unchanged.

That's just a "might be the easiest and eventually most tightly kept solution" suggestion, generally don't have a strong opinion on that.

@s1na
Copy link
Contributor Author

s1na commented Nov 4, 2019

manage here some array of the legacy HFs and then internally fetch from the sub folder - and pass this on to the VM and keep the VM code unchanged.

Just had a look and it seems the VM doesn't query based on HF, but queries directly for source files. I also have a tendency to keep this repo as only a thin wrapper and have the logic of parsing test cases in the using code (e.g. VM).

That said, we might be able to modify getTestsFromArgs which accepts the fork as an arg, and choose files/subdirs based on that. Then modify VM to use this API.

@holgerd77
Copy link
Member

@s1na Ok, just saw how you integrated, should we then leave this just "as is" and merge here? Given that there are so heavy changes in the submodule structure, I would give this at least a v1.3.0 for release?

@s1na
Copy link
Contributor Author

s1na commented Nov 12, 2019

@holgerd77 sorry I somehow missed this message. I'd be okay with merging like this. I integrated these new tests in the VM and it wasn't much work. Yeah I agree with v1.3.0.

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.

Thanks Sina, ok, then I'll approve here and prepare the release notes. Also just asked on the tests repo if it might be the time for another beta or eventually a final Istanbul release.

@holgerd77 holgerd77 merged commit 7707c42 into master Nov 12, 2019
@holgerd77 holgerd77 deleted the istanbul-tests branch November 12, 2019 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants