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

[fix] #3928: Make failing the wasm tests actually fail the CI #3933

Merged

Conversation

DCNick3
Copy link
Contributor

@DCNick3 DCNick3 commented Sep 28, 2023

Description

Add a new iroha_wasm_test_runner tool. It mostly duplicates the webassembly-test-runner, but actually returns a non-zero exit code if there are any test failures.

This tool is expected to be installed on the developer's system with cargo install. It's not going to change often, so I don't think there's a risk of it going out-of-sync with the state of the repo.

Also fix the evaluate_expression test, which was never passing before due to a typo.

Linked issue

Closes #3928

Benefits

  • Actually makes the CI check that the wasm test

Alternatives

An alternative would be to continue to use the upstream test runner when/if matklad/webassembly-test#1 gets merged, but I wouldn't bet on it.

Checklist

  • make sure CI passes

@coveralls
Copy link

coveralls commented Sep 28, 2023

Pull Request Test Coverage Report for Build 6416151955

  • 0 of 46 (0.0%) changed or added relevant lines in 1 file are covered.
  • 6351 unchanged lines in 108 files lost coverage.
  • Overall coverage decreased (-0.9%) to 58.572%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tools/wasm_test_runner/src/main.rs 0 46 0.0%
Files with Coverage Reduction New Missed Lines %
cli/src/main.rs 1 0.0%
cli/src/samples.rs 1 0.0%
cli/src/torii/pagination.rs 1 98.9%
config/base/src/runtime_upgrades.rs 1 51.72%
config/src/torii.rs 1 96.67%
core/src/smartcontracts/isi/block.rs 1 87.5%
crypto/src/merkle.rs 1 96.23%
data_model/src/domain.rs 1 48.25%
logger/src/lib.rs 1 95.15%
config/src/lib.rs 2 0.0%
Totals Coverage Status
Change from base Build 5423219773: -0.9%
Covered Lines: 21698
Relevant Lines: 37045

💛 - Coveralls

mversic
mversic previously approved these changes Sep 28, 2023
Copy link
Contributor

@mversic mversic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you also opened a PR to webassembly-test-runner repo. Do you think we should use it when your fix is merged?

@DCNick3
Copy link
Contributor Author

DCNick3 commented Sep 28, 2023

Yeah, I think it's fine to use it when/if it'll get merged. But, AFAIK, matklad is not doing any rust stuff recently, so it might be a while.

@DCNick3
Copy link
Contributor Author

DCNick3 commented Sep 28, 2023

The iroha_wasm::tests::execute_instruction test fails when CARGO_INCREMENTAL is 0, but passes when it's 1. I am not sure how to even approach debugging this...

@mversic
Copy link
Contributor

mversic commented Sep 28, 2023

@Arjentix can you help Nikita debug this?

@Arjentix
Copy link
Contributor

Will try

@DCNick3
Copy link
Contributor Author

DCNick3 commented Sep 28, 2023

I have some progress, but not much: it's probably a result of some unsoundness, or a compiler bug, as modifying the code (like inserting a call to a log function) can make the failure disappear. It also seems to occur in release regardless of whether the incremental compilation is used

@DCNick3
Copy link
Contributor Author

DCNick3 commented Sep 28, 2023

It's a double free caused by the confusion abt whether the function should take ownership.

While debugging this, I've added ad-hoc logging capabilities to the test runner. Not sure if this logger should stay. (It would allow us to move back to the upstream webassembly-test-runner, as my PR was merged)

@DCNick3 DCNick3 force-pushed the iroha-wasm-test-runner branch 2 times, most recently from d0fa213 to 4b9d8ac Compare September 29, 2023 11:34
@DCNick3
Copy link
Contributor Author

DCNick3 commented Sep 29, 2023

Removed the ad-hoc logging system & made another PR to the upstream (for printing the backtrace of test failures). If it gets merged I'll get rid of the iroha_wasm_test_runner.

@mversic
Copy link
Contributor

mversic commented Sep 29, 2023

Removed the ad-hoc logging system & made another PR to the upstream (for printing the backtrace of test failures). If it gets merged I'll get rid of the iroha_wasm_test_runner.

ok, since this PR is unlikely to conflict with any of the current work we can wait a few days before merging

@DCNick3 DCNick3 force-pushed the iroha-wasm-test-runner branch from 4b9d8ac to 0a66b3e Compare September 29, 2023 13:11
6r1d
6r1d previously approved these changes Sep 29, 2023
Copy link
Contributor

@6r1d 6r1d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for a detailed explanation, hopefully we can rebuild the image soon :-)

@mversic mversic self-assigned this Oct 2, 2023
Dockerfile.build Outdated Show resolved Hide resolved
@DCNick3
Copy link
Contributor Author

DCNick3 commented Oct 2, 2023

Reverted to hosting the wasm runner tool in monorepo, as the upstream would still be effectively only maintained by us

@mversic mversic requested a review from 6r1d October 3, 2023 07:29
tools/wasm_test_runner/src/main.rs Show resolved Hide resolved
The `log` and `dbg` functions do not take the pointer ownership, but their mock versions used for testing did

Signed-off-by: Nikita Strygin <[email protected]>
@DCNick3 DCNick3 force-pushed the iroha-wasm-test-runner branch from f61aaf0 to 47b2998 Compare October 5, 2023 07:48
@DCNick3 DCNick3 merged commit f3dc621 into hyperledger-iroha:iroha2-dev Oct 6, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants