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

fix(iota-benchmark): fix test_upgrade_compatibility #3732

Merged
merged 7 commits into from
Oct 29, 2024
Merged

fix(iota-benchmark): fix test_upgrade_compatibility #3732

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6f42e6d
Fix test_upgrade_compatibility
bingyanglin Oct 29, 2024
ad01ffe
Merge branch 'develop' into core-node/fix/upgrade_test
bingyanglin Oct 29, 2024
5d91a79
Update crates/iota-benchmark/tests/simtest.rs
bingyanglin Oct 29, 2024
60e3a3e
Modify the ProtocolVersion::MAX_ALLOWED
bingyanglin Oct 29, 2024
5fb7204
Revert the MAX_ALLOWED to avoid breaking other testings
bingyanglin Oct 29, 2024
24ad1a9
Use MAX + 1 for protocol upgrade testing for now
bingyanglin Oct 29, 2024
9956f64
Merge branch 'develop' into core-node/fix/upgrade_test
bingyanglin Oct 29, 2024
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
2 changes: 1 addition & 1 deletion crates/iota-benchmark/tests/simtest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,7 @@ mod test {
}

async fn test_protocol_upgrade_compatibility_impl() {
let max_ver = ProtocolVersion::MAX.as_u64();
let max_ver = ProtocolVersion::MAX.as_u64() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work as well? Then we don't have to remove the +1 in the future.

Suggested change
let max_ver = ProtocolVersion::MAX.as_u64() + 1;
let max_ver = ProtocolVersion::MAX_ALLOWED.as_u64();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it does not pass.

Copy link
Contributor Author

@bingyanglin bingyanglin Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@muXxer @alexsporn I changed the MAX_ALLOWED to be MAX + 1 to pass the upgrade testing in 60e3a3e.

Note that SUI also uses

    #[cfg(msim)]
    pub const MAX_ALLOWED: Self = Self(MAX_PROTOCOL_VERSION + 1);

Copy link
Contributor

Choose a reason for hiding this comment

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

NOOO that breaks another test I fixed by doing +4. 😢

Copy link
Contributor Author

@bingyanglin bingyanglin Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@muXxer got it. Sorry for that. Then how about we use MAX + 1 inside test_upgrade_compatibility now and add TODO comments, so we can remember to modify it when we have really version 2? Also we keep MAX_ALLOWED to be +4.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, where did you see it doesn't work? The CI was green for this test, or am I confused?
grafik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or I am fine with simply ignore it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed with @muXxer in the call we decided to use MAX + 1 for now and add TODO comments at 24ad1a9

Copy link
Contributor Author

@bingyanglin bingyanglin Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

btw, where did you see it doesn't work? The CI was green for this test, or am I confused?

Sorry just saw this message. It was lagged because I didn't refresh this page. It failed twice in my machine. I reran the sim-testing GA for the commit to see if I missed something.

let manifest = iota_framework_snapshot::load_bytecode_snapshot_manifest();

let Some((&starting_version, _)) = manifest.range(..max_ver).last() else {
Expand Down
Loading