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

PVF: improve test ergonomics #1724

Closed
2 tasks
mrcnski opened this issue Sep 27, 2023 · 0 comments · Fixed by #1771
Closed
2 tasks

PVF: improve test ergonomics #1724

mrcnski opened this issue Sep 27, 2023 · 0 comments · Fixed by #1771

Comments

@mrcnski
Copy link
Contributor

mrcnski commented Sep 27, 2023

Due to the retiring of puppet workers, the PVF tests now depend not on fake workers but on the actual real workers. This is better as we now test the real thing, on the other hand we now have to remember to manually rebuild the actual workers before running tests. There are two issues with this:

  1. The error when you forget to build the workers is not clear at all: Tracking/limiting memory allocator #1192 (comment)
  2. After Make the node version independent of the crate version #1495 the workers now depend on polkadot-cli (for NODE_VERSION), which itself transitively depends on the PVF system. This means that if you make a change in PVF and need to test it, all of polkadot-cli gets rebuilt when you build the workers.

Couple of ideas:

  1. Maybe the build script of PVF can make sure the workers were built with the current git commit hash?
  2. Move NODE_VERSION into some shared location. Maybe something like a .env file or some equivalent that works with cargo, so an env var gets set which we can use at compile-time.
    • After some testing, it seems that even if the workers pull in nothing from pvf, they always depend on it. Cargo considers them to have this dependency regardless, because they are bin targets in the same crate as polkadot and share dependencies with it. To fix it, they need to be in their own crate instead of sharing a crate with polkadot.

cc @s0me0ne-unkn0wn

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 a pull request may close this issue.

1 participant