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

Add a nightly build to circleci which runs even the slow blockchain tests #1588

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lithp
Copy link
Contributor

@lithp lithp commented Dec 13, 2018

Working off https://github.com/ethereum/py-evm/pull/1577/files#r240997519, it might be nice to run the SLOWEST_TESTS more often than never. I'm not sure if this works, I wasn't able to get it to run locally, circleci build --job py36-all-blockchain-tests kept failing with out of disk space errors.

Does anyone know of a better way to test it than merging and waiting for nightfall like this adorable little owl?

Cute Animal Picture

owl

@lithp lithp changed the title Add a nightly build which runs even the slow blockchain tests Add a nightly build to circleci which runs even the slow blockchain tests Dec 13, 2018
@carver
Copy link
Contributor

carver commented Dec 13, 2018

Does anyone know of a better way to test it than merging and waiting for nightfall like this adorable little owl?

I feel about as knowledgeable about circle CI cron jobs as that owl looks.

Here is a potentially bad idea: what if we only ran the slowest tests in the nightly job? Even if we do that, I'm a bit worried that the tests will fail based on memory issues. I feel mildly confident that they will if we try to run everything plus the slow tests. Even if we only run slow tests, we might have to break them up further into smaller jobs to convince circle to run them.

@pipermerriam
Copy link
Member

Anyone have experience setting up and running a Jenkins server? I think our best route to accomplish this would be having the devops team provisioning a more powerful machine that we can run these more intensive tests on (and maybe the benchmarks too since that machine is likely to have a more stable CPU profile.

@pipermerriam
Copy link
Member

Might also be worth looking into whether https://github.com/features/actions can help us with this.

@veox
Copy link
Contributor

veox commented Dec 13, 2018

Heads up!

PR #1579 pulls in tests that routinely consume 3 GiB of RAM when exec'd in a single process, or ~ 8 GiB at peak when allowed to run in parallel. CircleCI crashes for me when using Python 3.6.

These are now certainly listed in SLOWEST_TESTS. There are several (3?..), haven't pinpointed exactly which, but likely something from the top of these 5 pytest runs.

@veox
Copy link
Contributor

veox commented Dec 13, 2018

(Unrelated to the above.)

Note that there are other tests that aren't run, which are not in SLOWEST_TESTS:

def blockchain_fixture_mark_fn(fixture_path, fixture_name, fixture_fork):
fixture_id = (fixture_path, fixture_name)
if fixture_path.startswith('bcExploitTest'):
return pytest.mark.skip("Exploit tests are slow")
elif fixture_path == 'bcWalletTest/walletReorganizeOwners.json':
return pytest.mark.skip("Wallet owner reorganization tests are slow")
elif fixture_id in INCORRECT_UPSTREAM_TESTS:
return pytest.mark.xfail(reason="Listed in INCORRECT_UPSTREAM_TESTS.")
elif 'stTransactionTest/zeroSigTransa' in fixture_path:
return pytest.mark.skip("EIP-86 not supported.")
elif fixture_id in SLOWEST_TESTS:
if should_run_slow_tests():
return
else:
return pytest.mark.skip("Skipping slow test")
elif 'stQuadraticComplexityTest' in fixture_path:
return pytest.mark.skip("Skipping slow test")

Perhaps the function can be reworked to skip/xfail unsupported/incorrect, then check should_run_slow_tests(), then run if so (or not if not).

@lithp
Copy link
Contributor Author

lithp commented Dec 14, 2018

Opened #1606 to help reduce memory pressure. Hopefully it's enough to stop CircleCI from crashing!

Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

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

Let's see a run of this working (maybe by temporarily adding it to the main workflow) before we merge the change to the nightly.

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

Successfully merging this pull request may close these issues.

4 participants