PVF: Add worker check during tests and benches#1771
Conversation
Also: - Use an env var for the node version to avoid unnecessary dependencies on polkadot-cli - Add a benchmark for preparation through the host
s0me0ne-unkn0wn
left a comment
s0me0ne-unkn0wn
left a comment
There was a problem hiding this comment.
Wow, it's an impressive amount of checks for dev-only code! Nothing has caught my eye, looks good!
| panic!("ERROR: Workers do not exist or are not executable. Workers directory: {:?}", workers_path); | ||
| } | ||
|
|
||
| let node_version = env!("POLKADOT_NODE_VERSION"); |
There was a problem hiding this comment.
So this is now the reason the version is put into an env variable?
|
|
||
| // We don't want to check against the commit hash because we'd have to always rebuild | ||
| // the calling crate on every commit. | ||
| eprintln!("WARNING: Workers match the node version, but may have changed in recent commits. Please rebuild them if anything funny happens. Workers path: {workers_path:?}"); |
There was a problem hiding this comment.
You already print this here. So, this doesn't really make any sense to compare the version. If you really want to make it more "safe", you can call here cargo build to ensure the worker are there.
There was a problem hiding this comment.
Well, this warning won't show up unless -- --nocapture is passed. I left it because it's better than nothing I guess. I think it's still useful to separately check for worker existence and that they're on the same version.
If you really want to make it more "safe", you can call here cargo build to ensure the worker are there.
You mean cargo build in build.rs, every time the version changes?
There was a problem hiding this comment.
No, I mean this function does this at "runtime". This is only being used for testing stuff. So, it could ensure that the workers are up to date. It would also make testing easier, as people would not be required to first run cargo build after finding out that the workers are not there.
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
Would still like that get_and_check_worker_paths just calls cargo build to provide some better UX for tests. However, this isn't a real blocker as the current behavior isn't changed.
|
I'm considering removing the benches in favor of the one here: https://github.com/paritytech/polkadot-sdk/pull/1192/files#diff-50fee62320022788db79b0efec4a3dd9e9c7060efbcb6af6a4ce27f31392a395. For the measurement I want to get (time of an execution job), it would be better to add a metric and get it by running zombienet. Because right now execution always returns early due to an invalid candidate so I don't get an accurate stat. Edit: oh, we already have such a metric ( |
…ing-tests' into mrcnski/pvf-add-worker-check-during-tests
|
The CI pipeline was cancelled due to failure one of the required jobs. |
* tsv-disabling: (36 commits) Removed TODO from test-case for hard-coded delivery fee estimation (#2042) Expose collection attributes from `Inspect` trait (#1914) `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897) [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023) Sort the benchmarks before listing them (#2026) publish pallet-root-testing (#2017) Contracts: Add benchmarks to include files (#2022) Small optimisation to `--profile dev` wasm builds (#1851) basic-authorship: Improve time recording and logging (#2010) Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815) [ci] Run check-rust-feature-propagation in pr and master (#2012) Improve features dev-ex (#1831) Remove obsolete comment. (#2008) Refactor candidates test in paras_inherent (#2004) PVF: Add worker check during tests and benches (#1771) Bump actions/setup-node from 3.8.1 to 4.0.0 (#1997) polkadot: enable tikv-jemallocator/unprefixed_malloc_on_supported_platforms (#2002) Make `IdentityInfo` generic in `pallet-identity` (#1661) Ensure correct variant count in `Runtime[Hold/Freeze]Reason` (#1900) `CheckWeight`: Add more logging (#1996) ...
6 participants
Improves test ergonomics. Please see #1724.
Also:
Related
Closes #1724