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

CSUB-312: Conditionally panic! in migrations when testing #820

Merged
merged 3 commits into from
Jan 6, 2023

Conversation

atodorov
Copy link
Collaborator

so that future tests would crash if unexpected situations happen and we would have to inspect the crashes and figure out how to deal with potential corner cases

@nathanwhit, @CAGS295 - what would happen if even despite tests such panic happens when migrating a mainnet node ?

@github-actions
Copy link

For full LLVM coverage report click here!

@atodorov atodorov marked this pull request as ready for review December 19, 2022 11:53
@CAGS295
Copy link
Contributor

CAGS295 commented Dec 19, 2022

I haven't explored yet what happens under panics. We need to consider what happens when the scheduled upgrade fails. I suggest having a feature option for testing and otherwise never panic.

@nathanwhit
Copy link
Contributor

nathanwhit commented Dec 19, 2022

If the migration panics, the chain is effectively bricked. No new blocks will be produced, and the only "fix" is to get a majority of the network to revert the block that triggered the update.

So yeah, I agree with Carlos. If we do want it to panic on failure, the panic should be gated for testing.

@atodorov atodorov force-pushed the CSUB-312-panic-in-migrations branch 2 times, most recently from 39894de to faaec52 Compare December 20, 2022 09:54
@atodorov atodorov changed the title CSUB-312: Replace log::warn! with panic! in migrations CSUB-312: Conditionally panic! in migrations when testing Dec 20, 2022
@atodorov atodorov force-pushed the CSUB-312-panic-in-migrations branch 6 times, most recently from 1e2ff98 to d054390 Compare December 20, 2022 21:11
@atodorov
Copy link
Collaborator Author

Failing job in CI if we're using log::warn! directly instead of the warn_or_panic! - https://github.com/gluwa/creditcoin/actions/runs/3744124997/jobs/6357132834

PASS -> https://github.com/gluwa/creditcoin/actions/runs/3744195353/jobs/6357285397 after the current changes.

@atodorov atodorov force-pushed the CSUB-312-panic-in-migrations branch 3 times, most recently from f60019f to 3ad0575 Compare December 20, 2022 21:24
so that future tests would crash if unexpected situations happen
and we would have to inspect the crashes and figure out how to deal with
potential corner cases
@atodorov atodorov force-pushed the CSUB-312-panic-in-migrations branch from 3ad0575 to 234c82b Compare January 3, 2023 11:23
@atodorov atodorov requested a review from CAGS295 January 3, 2023 11:26
@codecov-commenter
Copy link

Codecov Report

Merging #820 (234c82b) into dev (80ed87a) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##              dev     #820   +/-   ##
=======================================
  Coverage   78.05%   78.05%           
=======================================
  Files          66       66           
  Lines       10609    10609           
=======================================
  Hits         8281     8281           
  Misses       2328     2328           
Impacted Files Coverage Δ
pallets/creditcoin/src/lib.rs 81.70% <ø> (ø)
pallets/creditcoin/src/migrations/v6.rs 89.21% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nathanwhit nathanwhit merged commit 3cb541a into dev Jan 6, 2023
@nathanwhit nathanwhit deleted the CSUB-312-panic-in-migrations branch January 6, 2023 17:47
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

4 participants