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

Run "slow" tests as part of pull request checks #2322

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

Conversation

tautschnig
Copy link
Member

Description of changes:

The execution of this check takes less than 15 minutes, which is well below our overall CI time for pull requests (around 45 minutes). Making these tests part of pull request checks will ensure we catch problems early.

Resolved issues:

n/a

Related RFC:

n/a

Call-outs:

n/a

Testing:

  • How is this change tested? Part of this pull request.

  • Is this a refactor change? Sort of - refactoring of CI.

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • n/a Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@zhassan-aws
Copy link
Contributor

Thanks @tautschnig. As far as I recall, the slow tests suite was introduced for the purpose of running regressions that take too long for CI. If particular tests are no longer time consuming, I think we should move them to be part of the regular regression suite. But I think we should keep the slow tests workflow for cases that take too long. Alternatively, we can any long running tests tests to the performance regression suite so that we can track any performance improvement.

@zhassan-aws
Copy link
Contributor

Also worth mentioning that there are many harnesses in the tokio-proofs regression that are currently disabled because they ran for more than 2 hours. I just tried one of them and it completed in 3 seconds (after updating the unwind to 12). So we should revisit them and check which ones we can enable.

Make sure we catch problems as they appear, even when that is just after
merging into main: slow tests might eventually take too long to run for
them to be part of pull-request CI, so delay the execution until after
merging.
@tautschnig
Copy link
Member Author

It seems the tests themselves (tokio-proofs) take between 2 to 3 minutes, but then I don't know what those tests are specifically testing - I could just move them to a new folder under tests/ (tests/tokio-proofs), but I don't know whether that's justified. Also, in doing so tests/slow would be left empty, so I'd have to add a dummy file to keep the folder in git.

For the moment I took a different approach and moved the runs from "Nightly" to run-on-push-to-main: there is nothing in these tests where a different day would make a difference, it's all about the code we have in the repository. So we should detect problems as early as possible, and if we think that (eventually) those tests will take too long for pull-request CI then let's run them immediately after the merge so that we know as soon as possible when we messed up.

Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

I agree with @zhassan-aws. We should move those harnesses out of the slow folder. If that leaves us with no slow harness, then we should change this job to not fail if there is no test.

@tautschnig tautschnig self-assigned this Apr 27, 2023
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