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

New testStateDev test run command to allow for parallel Constantinople testing #317

Merged
merged 1 commit into from Jul 23, 2018

Conversation

holgerd77
Copy link
Member

No description provided.

@holgerd77 holgerd77 requested a review from axic July 23, 2018 11:55
@holgerd77
Copy link
Member Author

holgerd77 commented Jul 23, 2018

@axic Ok, this would allow for in-parallel Constantinople test runs.

There is no way of not adding Constantinople to the supportedHardforks list if tests should be able to run. In my opinion this is ok if it's clearly documented. If you don't like that, there is still the alternative that people have to re-add this on every Constantinople PR, which would be more inconvenient (at the end it would have to be removed again before merging), but possible.

@holgerd77
Copy link
Member Author

@axic No way of "not adding" of course, have updated this in the comment.

@holgerd77
Copy link
Member Author

@axic Hmm, I think I'll rename this from test_state_dev to test_state_constantinople, then it is more transparent why this is failing and we don't have to re-explain this all the time to external contributors.

@holgerd77 holgerd77 force-pushed the add-constantinople-state-test-runs branch from b9a6d1b to 2d2a95b Compare July 23, 2018 12:03
@coveralls
Copy link

coveralls commented Jul 23, 2018

Coverage Status

Coverage remained the same at 85.531% when pulling 25bbd1c on add-constantinople-state-test-runs into 41b5a0b on master.

@axic
Copy link
Member

axic commented Jul 23, 2018

I guess we can select test_state_constantinople as allowed to fail in github.

@axic
Copy link
Member

axic commented Jul 23, 2018

I see that's already done. I think the only comment is that I'd rename test_state to test_state_byzantium.

@holgerd77 holgerd77 force-pushed the add-constantinople-state-test-runs branch from 2d2a95b to 25bbd1c Compare July 23, 2018 18:41
@holgerd77 holgerd77 merged commit a7275c7 into master Jul 23, 2018
@holgerd77 holgerd77 deleted the add-constantinople-state-test-runs branch July 23, 2018 18:50
@holgerd77
Copy link
Member Author

Review done informally by @axic, merged.

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.

None yet

4 participants