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.

Sign up for GitHub

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

Jump to bottom

fix(pallet-benchmarking): split test functions in v2 #3574

Merged
merged 7 commits into from
Mar 8, 2024
Merged

fix(pallet-benchmarking): split test functions in v2 #3574

merged 7 commits into from
Mar 8, 2024

Conversation

pandres95
Copy link
Contributor

Closes #376

@pandres95 pandres95 requested a review from a team as a code owner March 5, 2024 09:43
@pandres95 pandres95 marked this pull request as draft March 5, 2024 09:43
@pandres95 pandres95 force-pushed the feat/benchmarking-v2-split-bench-tests branch from 0944d5d to 02eef67 Compare March 5, 2024 17:55
@pandres95 pandres95 marked this pull request as ready for review March 6, 2024 05:22
@pgherveou pgherveou mentioned this pull request Mar 6, 2024
Merged
@kianenigma kianenigma requested a review from gupnik March 6, 2024 12:04
@paritytech-review-bot paritytech-review-bot bot requested a review from a team March 6, 2024 12:04
@pandres95 pandres95 force-pushed the feat/benchmarking-v2-split-bench-tests branch 2 times, most recently from 72c8efc to 96cfc1d Compare March 6, 2024 16:29
@pandres95 pandres95 force-pushed the feat/benchmarking-v2-split-bench-tests branch from 96cfc1d to 5312b58 Compare March 6, 2024 16:31
@pandres95 pandres95 requested review from pgherveou and ggwpez March 6, 2024 16:32
@pgherveou
Copy link
Contributor

LGTM, the only difference with v1 now is that the tests are nested into the benchmarks mod

@pandres95
Copy link
Contributor Author

pandres95 commented Mar 6, 2024 •
edited
Loading

Correct! That'd be the only difference.

However, I can make them outside, let me try.
Updates:

  • Yup, it's possible. Had to change the visibility of the test_benchmark_$name functions to pub(super), but it's perfectly possible
  • According to clippy, seems like there are more things than the mere benchmarking methods that would need to be exposed, so I'll just go back to the previous state.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5458546

@pandres95 pandres95 force-pushed the feat/benchmarking-v2-split-bench-tests branch from 359c7d1 to 5312b58 Compare March 6, 2024 17:38
@ggwpez
Copy link
Member

ggwpez commented Mar 6, 2024 •
edited
Loading

LGTM, the only difference with v1 now is that the tests are nested into the benchmarks mod

I think that actually makes more sense then if they would teleport themselves outside. But yes, its a difference.

ggwpez
ggwpez approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice!
Since it is a change in default behaviour, please add a minimal prdoc https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/prdoc.md

substrate/frame/support/procedural/src/benchmark.rs Show resolved Hide resolved
@ggwpez
Copy link
Member

ggwpez commented Mar 7, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Mar 7, 2024 •
edited
Loading

@ggwpez https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5465411 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-1204ed78-a517-47f5-a7ca-0bd6ffa4e9cc to cancel this command or bot cancel to cancel all commands in this pull request.

@ggwpez ggwpez added the T12-benchmarks This PR/Issue is related to benchmarking and weights. label Mar 7, 2024
@command-bot
Copy link

command-bot bot commented Mar 7, 2024

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5465411 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5465411/artifacts/download.

@pandres95
Copy link
Contributor Author

@ggwpez the prdoc is ready for reviewing.

@ggwpez ggwpez added this pull request to the merge queue Mar 7, 2024
Merged via the queue into paritytech:master with commit f4fbdde Mar 8, 2024
129 of 130 checks passed
@pandres95 pandres95 deleted the feat/benchmarking-v2-split-bench-tests branch March 10, 2024 03:06
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@pandres95
fix(pallet-benchmarking): split test functions in v2 (paritytech#3574)
53dfdef 
Closes paritytech#376

---------

Co-authored-by: command-bot <>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/decoded-2024-sponsorship-for-active-community-members/7654/71

This was referenced Jun 5, 2024
Closed
Closed
Closed
Closed
@girazoki girazoki mentioned this pull request Jun 7, 2024
Merged
4 tasks
@Aideepakchaudhary Aideepakchaudhary mentioned this pull request Oct 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@pgherveou pgherveou pgherveou approved these changes

@gupnik gupnik Awaiting requested review from gupnik

Assignees
No one assigned
Labels
T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bench syntax v2: run test in single functions
5 participants
@pandres95 @pgherveou @paritytech-cicd-pr @ggwpez @Polkadot-Forum