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.

Sign up for GitHub

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

Jump to bottom

Fix libwasmvm tests for windows and check it in CI #338

Closed
wants to merge 4 commits into from
Closed

Fix libwasmvm tests for windows and check it in CI #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a90f94
Fix libwasmvm tests for windows and check it in CI
devashishdxt Jul 14, 2022
7b64f9f
Install specific rustup version
devashishdxt Jul 14, 2022
671dfd8
Install rustup 1.24.3 on windows
devashishdxt Jul 14, 2022
4a10447
Provide checksums manually for windows rustup installation
devashishdxt Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
version: 2.1

orbs:
win: circleci/windows@4.1.1

jobs:
# All checks on the codebase that can run in parallel to build_shared_library
libwasmvm_sanity:
Expand Down Expand Up @@ -182,6 +185,28 @@ jobs:
command: make test-safety
- run: make build-go

rust-unit-tests-windows:
executor:
name: win/default
shell: powershell.exe
environment:
CARGO_NET_GIT_FETCH_WITH_CLI: "true"
steps:
- checkout
- run:
name: Install Rustup
command: choco install rustup.install --checksum 79442f66a969a504febda49ee9158a90ad9e94913209ccdca3c22ef86d635c31 --checksum64 2220DDB49FEA0E0945B1B5913E33D66BD223A67F19FD1C116BE0318DE7ED9D9C
- run:
name: Install Rust
command: rustup default 1.59.0
- run:
name: Show Rust version information
command: rustc --version; cargo --version; rustup --version
- run:
name: Run unit tests
working_directory: libwasmvm
command: cargo test

test_alpine_build:
machine:
image: ubuntu-2004:202101-01
Expand Down Expand Up @@ -283,6 +308,7 @@ workflows:
- tidy-go
- format-scripts
- lint-scripts
- rust-unit-tests-windows
- build_shared_library:
filters: # required since other jobs with tag filters require this one
tags:
Expand Down
8 changes: 8 additions & 0 deletions libwasmvm/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions libwasmvm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ thiserror = "1.0"
hex = "0.4"

[dev-dependencies]
assert_approx_eq = "1.1.0"
cfg-if = "1.0.0"
serde = { version = "1.0.103", default-features = false, features = ["derive"] }
tempfile = "3.1.0"

Expand Down
31 changes: 18 additions & 13 deletions libwasmvm/src/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,13 @@ mod tests {
assert!(cache_ptr.is_null());
assert!(error_msg.is_some());
let msg = String::from_utf8(error_msg.consume().unwrap()).unwrap();
assert_eq!(msg, "Error calling the VM: Cache error: Error creating directory broken\u{0}dir/state: data provided contains a nul byte");
Copy link

Choose a reason for hiding this comment

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

this error message isn't committed anywhere in the CW state, right?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I'll check.

Copy link

Choose a reason for hiding this comment

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

it's potentially also for third-party uses... for example, there was that issue in ibc-go where it committed the error message from Cosmos SDK and the message changed in Cosmos SDK seemingly non-breaking patches (within Cosmos SDK, it wasn't being committed to state anywhere).
but not sure whether wasmvm or wasmd would be used in a similar manner -- @webmaster128 @ethanfrey

Copy link
Author

Choose a reason for hiding this comment

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

This error only happens when initializing cache. In wasmd, init_cache function will get called here during keeper creation. So, I don't think it gets committed to CW state anywhere.

Copy link
Author

Choose a reason for hiding this comment

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

App will panic if this error happens.

Copy link

Choose a reason for hiding this comment

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

ok, that looks all right then -- I guess it's the similar case with https://github.com/CosmWasm/cosmwasm/pull/1369/files/a2c262d7b205e6aab448e24c790800f07ace2234#diff-a33cf9c4a456c8ddde19ce339a98e000023ad1dbdb34ade052cb401950dcc5daR537

cfg_if::cfg_if! {
if #[cfg(windows)] {
assert_eq!(msg, "Error calling the VM: Cache error: Error creating directory broken\u{0}dir\\state: strings passed to WinAPI cannot contain NULs");
} else {
assert_eq!(msg, "Error calling the VM: Cache error: Error creating directory broken\u{0}dir/state: data provided contains a nul byte");
}
}
}

#[test]
Expand Down Expand Up @@ -693,18 +699,17 @@ mod tests {
let mut error_msg = UnmanagedVector::default();
let metrics = get_metrics(cache_ptr, Some(&mut error_msg));
let _ = error_msg.consume();
assert_eq!(
metrics,
Metrics {
hits_pinned_memory_cache: 0,
hits_memory_cache: 0,
hits_fs_cache: 1,
misses: 0,
elements_pinned_memory_cache: 1,
elements_memory_cache: 0,
size_pinned_memory_cache: 5665691,
size_memory_cache: 0,
}
Copy link
Member

Choose a reason for hiding this comment

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

@uint are you aware of an implementation similar to this assert_approx_eq but with a relative epsilon? I don't want to say 5665691 +/- 665691 but 5665691 +/- 15%. We could use that for some gas tests as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Jakub or Bart (I don't remember) actually did that for Isotonic. It's easy to implement. I'd just write our own macro in cosmwasm and maybe open a PR to the assert_approx_eq crate.

Copy link
Member

Choose a reason for hiding this comment

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

That would be much appreciated!

Copy link
Member

Choose a reason for hiding this comment

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

This particular issue should be solved as part of #351

assert_eq!(metrics.hits_pinned_memory_cache, 0);
assert_eq!(metrics.hits_memory_cache, 0);
assert_eq!(metrics.hits_fs_cache, 1);
assert_eq!(metrics.misses, 0);
assert_eq!(metrics.elements_pinned_memory_cache, 1);
assert_eq!(metrics.elements_memory_cache, 0);
assert_eq!(metrics.size_memory_cache, 0);
assert_approx_eq::assert_approx_eq!(
i128::from(metrics.size_pinned_memory_cache),
5665691,
665691
);

// Unpin
Expand Down