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

Retire puppet workers #1449

Merged
merged 3 commits into from
Sep 11, 2023
Merged

Retire puppet workers #1449

merged 3 commits into from
Sep 11, 2023

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn commented Sep 7, 2023

Closes #583

After the separation of PVF worker binaries, dedicated puppet workers are not needed for tests anymore. The production workers can be used instead, avoiding some code duplication and decreasing complexity.

The changes also make it possible to further refactor the code to isolate workers completely.

Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

Really happy to see this!

This requires the workers to be built before running tests, right? Instructions should be added to the relevant READMEs, like we do for polkadot-test-service.

polkadot/node/core/pvf/common/src/worker/mod.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/tests/it/worker_common.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/tests/it/worker_common.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/tests/it/worker_common.rs Outdated Show resolved Hide resolved
@s0me0ne-unkn0wn s0me0ne-unkn0wn marked this pull request as ready for review September 10, 2023 10:13
@s0me0ne-unkn0wn
Copy link
Contributor Author

@mrcnski I've added a README to the PVF host integration tests directory. Not sure if it has to be added somewhere else, all the other tests seem good to go, but I may be missing something.

@s0me0ne-unkn0wn s0me0ne-unkn0wn added the T10-tests This PR/Issue is related to tests. label Sep 10, 2023
@mrcnski mrcnski mentioned this pull request Sep 10, 2023
1 task
Comment on lines +42 to +44
let mut workers_path = std::env::current_exe().unwrap();
workers_path.pop();
workers_path.pop();
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add this bit of logic to a helper function? it's being used from multiple places

@s0me0ne-unkn0wn s0me0ne-unkn0wn merged commit 2c8021f into master Sep 11, 2023
15 of 17 checks passed
@s0me0ne-unkn0wn s0me0ne-unkn0wn deleted the s0me0ne/retire-puppet-workers branch September 11, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T10-tests This PR/Issue is related to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retire puppet workers
4 participants