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

Attempt to speed up ATs #6948

Merged
merged 2 commits into from Apr 16, 2024
Merged

Attempt to speed up ATs #6948

merged 2 commits into from Apr 16, 2024

Conversation

siladu
Copy link
Contributor

@siladu siladu commented Apr 15, 2024

Attempt to revert long running AT times from 7e46889

Signed-off-by: Simon Dudley simon.dudley@consensys.net

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@siladu siladu changed the title Attempt to revert long running AT times from 7e46889 Attempt to speed up ATs Apr 15, 2024
@siladu siladu marked this pull request as ready for review April 15, 2024 05:44
@siladu siladu marked this pull request as draft April 15, 2024 05:45
This reverts commit 7e46889.

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@fab-10
Copy link
Contributor

fab-10 commented Apr 15, 2024

The commit that I did, is actually fixing the intent of the test, that was to stop the cluster after each single unit tests, and start with a new one every time, that intended behavior was broken, and had the side effect of keeping previously started Besu processes around for the duration of the test class, and so had the nice side effect that for all the test methods after the first a cluster was already up to respond to requests.
So instead of reverting to the wrong but faster state, I will prefer that we understand if the initial intends of restarting the cluster after each test methods was really needed and instead replace it with a restart each test class.
It is also worth to note that ATs in CI could be re-organized to balance the time.

Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

after a quick analysis, I found there are some steps needed to first port all the tests to Junit5, then identify test classes that do not need to have a cluster restart for every method and probably also a refactor of the base classes, so for the moment we can revert to the previous buggy but faster code, while implementing these pre tasks.

@siladu siladu marked this pull request as ready for review April 16, 2024 00:23
@siladu siladu enabled auto-merge (squash) April 16, 2024 00:25
@siladu siladu mentioned this pull request Apr 16, 2024
4 tasks
@siladu siladu merged commit b1d8554 into hyperledger:main Apr 16, 2024
42 checks passed
@siladu siladu deleted the speed-up-ats branch April 16, 2024 04:57
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
This reverts commit 7e46889.

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: amsmota <antonio.mota@citi.com>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
This reverts commit 7e46889.

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: amsmota <antonio.mota@citi.com>
macfarla pushed a commit to macfarla/besu that referenced this pull request Apr 26, 2024
This reverts commit 7e46889.

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
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.

None yet

2 participants