near / nearcore

Reference client for NEAR Protocol
https://near.org
GNU General Public License v3.0
2.31k stars 618 forks source link

fix(nayduck) - propagate test features to test contracts #11619

Closed wacban closed 3 months ago

wacban commented 3 months ago

The test fails in nayduck with "MethodNotFound" when trying to call the sleep method. This should only happen if the test contract is built without the test_features rust feature enabled.

I was able to reproduce it once locally. As soon as I added debug logs to the build.rs file in the near-test-contracts directory the test started passing. I suspect there is some race condition when building that causes the test_features to not be enabled. This is a lousy attempt to fix that issue by setting test_features wherever possible. I'm looking for hints on how to properly debug this.

nagisa commented 3 months ago

The only plausible reason I can think of for the race is if we end up with multiple concurrent compilations of the same contract, and those compilations write intermediate contracts into the same directory, so the final link ends up being racy and picks up object files non-deterministically.

Since we do not have multiple copies of the same crate, the only way I can think of for this to happen is multiple parallel invocations of cargo build -p {crate} --features {...} with different crates and/or features... In which case passing the feature sets from more places might not help, because ultimately you end up with with two different configurations regardless...

wacban commented 3 months ago

that didn't fix it: https://nayduck.nearone.org/#/test/47753 but at least I can repro in nayduck

nagisa commented 3 months ago

Your second command in nayduck is

cargo build -pgenesis-populate -prestaked -pnear-test-contracts --features nightly

i.e. it builds the test contracts separately without the test features enabled. Maybe that's it?

wacban commented 3 months ago

@nagisa The invocations look something like this:

cargo build -p neard --features nightly,test_features,rosetta_rpc
cargo build -p genesis-populate -p restaked -p near-test-contracts --features nightly,test_features

The genesis-populate and restaked packages don't have the test_features defined. Do you think it would help to have separate build command just for the near-test-contracts?

wacban commented 3 months ago

Your second command in nayduck is

cargo build -pgenesis-populate -prestaked -pnear-test-contracts --features nightly

i.e. it builds the test contracts separately without the test features enabled.

Ah this is a known issue in nayduck where the UI lives its own life. I checked with @andrei-near nayduck builder correctly includes the test features.

wacban commented 3 months ago

Also, even if only once, I did reproduce the issue locally.

nagisa commented 3 months ago

Looking at https://nayduck.nearone.org/#/build/424

I don't think any rebuilding is happening at all in the first place given the time it takes for the commands to run. It might have something to do with insufficient clobber information in our build.rs scripts or something else, but architecturally the problem begins with the fact that these wasm files end up in some random source directory, not the TARGET_DIR and then pytest framework goes on to grab the files from an unknown build of that crate so :shrug:

nagisa commented 3 months ago

The right-ish thing to do that's less error prone would be to:

  1. Have near-test-contracts not put things into source directory;
  2. Make a binary executable frontend of the crate that would output test contracts it built into a provided location for pytest to use (possibly outputting it to stdout?)
  3. Modify pytest to invoke said executable in order to obtain test contracts, rather than guesstimating where in the source code directories the intermediate artifacts might end up being stored...
wacban commented 3 months ago

Wow, thanks! This is a bit too large of a bite for me to chew before the next meeting so I'll come back to this some other time :)