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

ci: disabling most of the hardfork-specific tests #950

Merged
merged 2 commits into from
Nov 12, 2020

Conversation

evertonfraga
Copy link
Contributor

As promised, this PR disables most of the hardfork specific tests, to favor job concurrency.

It can be easily reverted, by un-commenting those lines.

@evertonfraga evertonfraga force-pushed the ci/reduce-amount-of-hf-test branch from 8909f12 to ffd1e04 Compare November 12, 2020 15:13
@codecov
Copy link

codecov bot commented Nov 12, 2020

Codecov Report

Merging #950 (5d8639d) into master (d8e7066) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 75.04% <ø> (ø)
blockchain 77.39% <ø> (ø)
common 92.11% <ø> (ø)
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
Copy link
Contributor Author

@jochem-brouwer double-checking: what are the most important HFs to test here? We can keep to 2 or 3 for brevity.

@holgerd77 After choosing the HFs, can I tweak the required status checks for master branch?

image

@jochem-brouwer
Copy link
Member

I'd say the most important are MuirGlacier (mainnet), Istanbul (latest hard fork w/h difficulty EIP -> MuirGlacier) since most networks run on one of both. All other forks are "outdated" so they don't need to be stress tested on every PR (except if changes are specific for that HF).

@@ -53,15 +53,15 @@ jobs:
matrix:
fork: [
'Berlin',
'MuirGlacier',
# 'MuirGlacier',
Copy link
Member

Choose a reason for hiding this comment

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

I'd say we also include MuirGlacier since this is what currently runs on mainnet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! the changes are in place.

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Yup, great!

@evertonfraga
Copy link
Contributor Author

I have updated the required checks according to the state and blockchain I have just changed.

Closing and reopening PR to try to see the Required Checks change in effect.

@evertonfraga evertonfraga reopened this Nov 12, 2020
@evertonfraga evertonfraga merged commit e8c5c54 into master Nov 12, 2020
@evertonfraga evertonfraga deleted the ci/reduce-amount-of-hf-test branch November 12, 2020 16:36
@holgerd77
Copy link
Member

All other forks are "outdated" so they don't need to be stress tested on every PR (except if changes are specific for that HF).

I would be careful with this, there are plenty of ways to break old HF code with lastest-fork-feature-adding PRs, we just had an example for this with the broken Constantinople tests fixed in #931. Should be ok though I would say with #951, we should nevertheless be aware of this and in doubt rather trigger the test runs for all the hardforks.

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