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

test: use WASM for backwards-compatibility tests #9173

Merged
merged 3 commits into from
Jun 12, 2023

Conversation

jakmeier
Copy link
Contributor

@jakmeier jakmeier commented Jun 12, 2023

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 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.
@jakmeier jakmeier requested a review from nagisa June 12, 2023 10:03
@jakmeier jakmeier requested a review from a team as a code owner June 12, 2023 10:03
@jakmeier jakmeier mentioned this pull request Jun 12, 2023
@@ -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
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@near-bulldozer near-bulldozer bot merged commit 863f712 into near:master Jun 12, 2023
@jakmeier jakmeier deleted the check_in_base_wasm branch June 12, 2023 12:34
nikurt pushed a commit that referenced this pull request Jun 13, 2023
`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)
@jakmeier jakmeier added the C-housekeeping Category: Refactoring, cleanups, code quality label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-housekeeping Category: Refactoring, cleanups, code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants