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

[aptos-stdlib] fix comparator tests #5694

Merged
merged 3 commits into from
Dec 11, 2022
Merged

Conversation

pause125
Copy link
Contributor

Description

The comarator is not suitable for Move integer types. Add more comment to describe this and fix the unittest.

Test Plan

No

@pause125 pause125 changed the title fix comparator tests [aptos-stdlib] fix comparator tests Nov 24, 2022
Copy link
Contributor

@movekevin movekevin left a comment

Choose a reason for hiding this comment

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

Thanks for the catch! This looks like a bug in the comparator that needs to be fixed, rather than just updating the test. But your intention here is to show proof that this is happening.

@pause125
Copy link
Contributor Author

pause125 commented Dec 7, 2022

Thanks for the catch! This looks like a bug in the comparator that needs to be fixed, rather than just updating the test. But your intention here is to show proof that this is happening.

I don't think this is bug. It ok too compare bcs serialized bytes. But the order of integer bcs serialized bytes will not be keep as the origin ones.

There is no need to add a new compare method for integer only, so maybe we can just rename the method to make it more clear?

@davidiw davidiw enabled auto-merge (squash) December 11, 2022 03:13

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@davidiw
Copy link
Contributor

davidiw commented Dec 11, 2022

we'll need to run this pre-commit run --all-files

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 386bdfc1794b4ac7bf7b9822e1d951ca7dd02e7a

performance benchmark with full nodes : 6380 TPS, 6207 ms latency, 13300 ms p99 latency,(!) expired 940 out of 2725440 txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 386bdfc1794b4ac7bf7b9822e1d951ca7dd02e7a

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 386bdfc1794b4ac7bf7b9822e1d951ca7dd02e7a (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7432 TPS, 5179 ms latency, 7300 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 386bdfc1794b4ac7bf7b9822e1d951ca7dd02e7a
compatibility::simple-validator-upgrade::single-validator-upgrade : 5039 TPS, 8245 ms latency, 10600 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 386bdfc1794b4ac7bf7b9822e1d951ca7dd02e7a
compatibility::simple-validator-upgrade::half-validator-upgrade : 4407 TPS, 9063 ms latency, 11800 ms p99 latency,no expired txns
4. upgrading second batch to new version: 386bdfc1794b4ac7bf7b9822e1d951ca7dd02e7a
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6352 TPS, 6249 ms latency, 10600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 386bdfc1794b4ac7bf7b9822e1d951ca7dd02e7a passed
Test Ok

@davidiw davidiw merged commit 47b8c2f into aptos-labs:main Dec 11, 2022
areshand pushed a commit to areshand/aptos-core-1 that referenced this pull request Dec 18, 2022
@Markuze Markuze mentioned this pull request Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants