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: Add best effort messages to random traffic canister #3108

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

stiegerc
Copy link
Contributor

@stiegerc stiegerc commented Dec 11, 2024

Adds support for best-effort calls on the random traffic canister. All arguments used to run the canister are collected in the Config now (except the random seed) since otherwise the number of update calls and individual parameters would get quite large.
Adds a new test where a local canister makes only best-effort calls, to itself and to a remote subnet. Checks that the local canister can be stopped even if the remote subnet stalls.

To change any parameter you can query the current config, then change it accordingly and send it back to the canister. The seed is excluded since reseeding the rng every time would reset the random sequence.

Additionally the records now contain a timeout if the call is best-effort and the time from making the call to receiving a reply is measured (for all calls).

@github-actions github-actions bot added the test label Dec 11, 2024
@stiegerc stiegerc marked this pull request as ready for review January 13, 2025 10:41
@stiegerc stiegerc requested review from a team as code owners January 13, 2025 10:41
rs/messaging/tests/memory_tests.rs Outdated Show resolved Hide resolved
rs/config/src/embedders.rs Outdated Show resolved Hide resolved
@alin-at-dfinity
Copy link
Contributor

Checks that the local canister can be stopped even if the remote subnet stalls.

Out of curiosity, why was it that the canister couldn't be stopped (IIRC) the other day?

rs/rust_canisters/random_traffic_test/src/lib.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/lib.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/lib.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/lib.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/main.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/main.rs Outdated Show resolved Hide resolved
rs/messaging/tests/memory_tests.rs Show resolved Hide resolved
rs/messaging/tests/memory_tests.rs Show resolved Hide resolved
Copy link
Member

@oggy-dfin oggy-dfin left a comment

Choose a reason for hiding this comment

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

LGTM overall, thanks! My main comment is related to the CDK version. Unfortunately, things have been shuffled around a bit in the CDK since you started writing this (for the better, hopefully!) and this would no longer compile with the HEAD of the next CDK version. While we could just pin it to the version you were using and leave it there, it'd suck to have to deal with the version from a random git commit in the future, so I'd propose we clean this up before merging (I don't think it should be a major change).

Furthermore, we should pin the commit in external_crates.bzl, otherwise things might break for someone else who needs to repin the Bazel dependencies (IIUC how the whole Bazel-Rust thing works).

bazel/external_crates.bzl Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/lib.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/lib.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/lib.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/main.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/main.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/main.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/main.rs Outdated Show resolved Hide resolved
rs/rust_canisters/random_traffic_test/src/main.rs Outdated Show resolved Hide resolved
rs/messaging/tests/memory_tests.rs Outdated Show resolved Hide resolved
@stiegerc
Copy link
Contributor Author

stiegerc commented Jan 16, 2025

Checks that the local canister can be stopped even if the remote subnet stalls.

Out of curiosity, why was it that the canister couldn't be stopped (IIRC) the other day?

I have a function tick() that ticks on both subnets and increments time. When I was trying to simulate one subnet stalling I used the vanilla tick() on StateMachine but this one doesn't increment time. So I just forgot that I have to increment time manually in this case and nothing was timed out.

edit: Well technically tick() on StateMachine does increment time, by 1 nanosecond. So the test would have passed it just would have taken billions of rounds :o)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants