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

Avoid running subsystem-benchmarks with nextest #3716

Closed
wants to merge 1 commit into from

Conversation

AndreiEres
Copy link
Contributor

Fixes #3704

@AndreiEres AndreiEres added R0-silent Changes should not be mentioned in any release notes T12-benchmarks This PR/Issue is related to benchmarking and weights. labels Mar 15, 2024
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

(no opinion on the code but it fixes my issue)

@bkchr
Copy link
Member

bkchr commented Mar 17, 2024

I don't get how this fix fixes the issue?

I also have the fundamental question, why do we have a benchmark registered as test? Registering the benchmark with harness = false should be achieve the same result?

Generally not sure why this is also not just wrapped into Criterion. I mean you have there logic for warmup etc that would all be done by criterion automatically.

@AndreiEres
Copy link
Contributor Author

@bkchr
Tests require subsystem-benchmarks feature, but they also depend on polkadot-availability-recovery with the same feature, so nextest runs them. That was the problem.

About why it's done by tests: they are a bit specific, so we need to implement them in that way without criterion.

@bkchr
Copy link
Member

bkchr commented Mar 18, 2024

About why it's done by tests: they are a bit specific, so we need to implement them in that way without criterion.

Okay fine, but you don't need to use criterion. If you use harness = false you basically have to write your main on your own, like you have done.

@AndreiEres
Copy link
Contributor Author

If you use harness = false you basically have to write your main on your own, like you have done.

Yes, but what do you mean?

Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

It's kind of odd to have 2 features in use now. I also don't really get why this fixes the issue.

@@ -51,3 +51,4 @@ required-features = ["subsystem-benchmarks"]

[features]
subsystem-benchmarks = []
subsystem-benchmarks-helpers = []
Copy link
Contributor

Choose a reason for hiding this comment

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

subsystem-benchmarks feature seems to no longer be used at all.

Copy link
Contributor Author

@AndreiEres AndreiEres Mar 18, 2024

Choose a reason for hiding this comment

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

It's in use. We require that feature to run regression tests. That way we don't mix normal and regression tests.

https://github.com/paritytech/polkadot-sdk/pull/3716/files#diff-ccb20ed2e9c6555ef9cd2b6e0905839764e8eac5da8970f170957da9cc9efc8fR46-R50

@sandreim
Copy link
Contributor

Can't the test just be skipped if feature not defined ?

@AndreiEres
Copy link
Contributor Author

Can't the test just be skipped if feature not defined ?

No, cargo test runs all test then.

@bkchr
Copy link
Member

bkchr commented Mar 18, 2024

Yes, but what do you mean?

I mean this: #3741

@bkchr
Copy link
Member

bkchr commented Mar 19, 2024

Closing in favor of: #3741

@bkchr bkchr closed this Mar 19, 2024
@bkchr bkchr deleted the AndreiEres/fix-bench-tests branch March 19, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Availability recovery bench hangs test runner
4 participants