-
Notifications
You must be signed in to change notification settings - Fork 618
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
test: use WASM for backwards-compatibility tests #9173
Conversation
`base_test_contract_rs.wasm` is a WASM used in our tests that is supposed to be compatible with the oldest version. That currently requires rustc < 1.70 or some hacks to use unstable cargo features. (`-Zbuild-std`) The easies way forward is to check in the WASM.
@@ -56,6 +56,15 @@ pub fn rs_contract() -> &'static [u8] { | |||
/// This is useful for tests that use a specific protocol version rather then | |||
/// just the latest one. In particular, protocol upgrade tests should use this | |||
/// function rather than [`rs_contract`]. | |||
/// | |||
/// Note: Unlike other contracts, this is not automatically build from source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we aren't checking-in all of the wasm files right now? This crate was clearly intended to make the tests easily extensible and more maintainable, but it seems like in doing so it has introduced worse maintenance hazards in other places of the codebase and therefore the net benefit is still negative. And this seemingly could have been any of the contracts from this crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most tests want to test only the latest protocol version and use the latest tool chain to build the WASM. For those, this crate seems to be quite useful still.
For the tests that need a fixed WASM, I am tentatively hopeful that we can delete them all once we have limited replayability.
I'd also argue that it's a good thing that some tests failed in this case (1.70 upgrade) because we do care about compatibility with the latest toolchain.
Overall, I'm not necessarily against the idea to check in all WASM files but I'm also not quite convinced it's a positive change right now.
`base_test_contract_rs.wasm` is a WASM used in our tests that is supposed to be compatible with the oldest version. That currently requires rustc < 1.70 or some hacks to use unstable cargo features. (`-Zbuild-std`) The easiest way forward is to check in a WASM built with the correct settings instead or relying on the nearcore toolchain version. This is a pre-requisite to land the 1.70 toolchain upgrade. (#9140)
base_test_contract_rs.wasm
is a WASM used in our tests that issupposed to be compatible with the oldest version.
That currently requires rustc < 1.70 or some hacks to
use unstable cargo features. (
-Zbuild-std
)The easiest way forward is to check in a WASM built with the correct
settings instead or relying on the nearcore toolchain version.
This is a pre-requisite to land the 1.70 toolchain upgrade. (#9140)