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

Tb/test marketplace upgrade #1880

Closed
wants to merge 14 commits into from
Closed

Tb/test marketplace upgrade #1880

wants to merge 14 commits into from

Conversation

tbro
Copy link
Contributor

@tbro tbro commented Aug 20, 2024

Test marketplace upgrade

if cf != chain_config_upgrade && height as u64 > stop_voting_view {
panic!("failed to upgrade chain config");
// Fail test if we've waited long enough
if height as u64 > stop_voting_view {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be inside the if statement if cf != chain_config_upgrade

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think If we exceed the number of configured views, we should fail the test. If chain_config gets upgraded after that number, still a failure. No?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if this is an accurate way to determine that chain config upgraded after that block height. The api call may take some time ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand I'm thinking we should avoid writing such mind bending tests. I think part of the reason we didn't catch the bug in the test is that it was so hard to understand. So maybe we can think of going in a different direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I think I should have added more comments to this test

@tbro tbro force-pushed the tb/test-marketplace-upgrade branch from 565bb5a to 46a6228 Compare August 21, 2024 12:27
tbro added 13 commits August 21, 2024 18:10
Test would never fail
panic if we exceed `stop_voting_view`
as it was height test was unreachable (except the case where chain
config had been updated). also added some comments.
sleep between iterations
Should be dropped automatically at the end of the test.
@tbro tbro force-pushed the tb/test-marketplace-upgrade branch from e1e87ef to 4f90ecf Compare August 21, 2024 15:17
[[upgrade]]
version = "0.3"
start_proposing_view = 1
stop_proposing_view = 10

[upgrade.marketplace]
[upgrade.marketplace.chain_config]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

marketplace genesis upgrade

@tbro tbro mentioned this pull request Aug 28, 2024
@tbro
Copy link
Contributor Author

tbro commented Aug 28, 2024

superseded by #1927

@tbro tbro closed this Aug 28, 2024
@Ancient123 Ancient123 deleted the tb/test-marketplace-upgrade branch August 29, 2024 15:40
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.

2 participants