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

Jump to bottom

CSUB-264: Minor follow up #826

Merged
merged 11 commits into from
Jan 18, 2023
Merged

CSUB-264: Minor follow up #826

merged 11 commits into from
Jan 18, 2023

Conversation

atodorov
Copy link
Collaborator

Description of proposed changes:

Practical tips for PR review & merge:

  • All GitHub Actions report PASS
  • Newly added code/functions have unit tests
    • Coverage tools report all newly added lines as covered
    • The positive scenario is exercised
    • Negative scenarios are exercised, e.g. assert on all possible errors
    • Assert on events triggered if applicable
    • Assert on changes made to storage if applicable
  • Modified behavior/functions - try to make sure above test items are covered
  • Integration tests are added if applicable/needed

@atodorov atodorov mentioned this pull request Dec 19, 2022
Merged
9 tasks
@atodorov atodorov changed the title CSUB-264: More testing related changes CSUB-264: Minor follow up Jan 5, 2023
@atodorov atodorov marked this pull request as ready for review January 5, 2023 16:40
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

For full LLVM coverage report click here!

@atodorov atodorov requested a review from CAGS295 January 9, 2023 13:57
@atodorov atodorov mentioned this pull request Jan 9, 2023
Merged
9 tasks
CAGS295
CAGS295 previously approved these changes Jan 10, 2023
View reviewed changes
@atodorov atodorov requested a review from AdaJane January 12, 2023 11:41
AdaJane
AdaJane previously approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@AdaJane AdaJane left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I still wonder if there's a better way to ensure the upgrade hooks are implemented (and correctly), but since the shell script is in direct response to a prior issue I don't think it's worth holding this up.

LGTM

CAGS295
CAGS295 previously approved these changes Jan 16, 2023
View reviewed changes
@atodorov atodorov dismissed stale reviews from CAGS295 and AdaJane via 7d4ae09 January 16, 2023 15:24
CAGS295
CAGS295 previously approved these changes Jan 16, 2023
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2023 •
edited
Loading

Codecov Report

Merging #826 (dcc774e) into dev (4e28108) will decrease coverage by 0.14%.
The diff coverage is 81.84%.

@@            Coverage Diff             @@
##              dev     #826      +/-   ##
==========================================
- Coverage   78.24%   78.11%   -0.14%     
==========================================
  Files          68       68              
  Lines       10726    10768      +42     
==========================================
+ Hits         8393     8411      +18     
- Misses       2333     2357      +24
Impacted Files Coverage Δ
pallets/creditcoin/src/lib.rs 81.70% <ø> (ø)
pallets/creditcoin/src/migrations/mod.rs 0.00% <0.00%> (ø)
pallets/difficulty/src/lib.rs 93.05% <ø> (ø)
primitives/src/lib.rs 93.33% <ø> (ø)
runtime/src/lib.rs 24.13% <0.00%> (+0.35%) ⬆️
pallets/creditcoin/src/migrations/v4.rs 82.92% <61.53%> (-11.20%) ⬇️
pallets/creditcoin/src/migrations/v5.rs 92.23% <75.00%> (-4.65%) ⬇️
pallets/creditcoin/src/migrations/v7.rs 86.53% <77.77%> (-9.02%) ⬇️
pallets/creditcoin/src/migrations/v6.rs 88.32% <83.33%> (-0.89%) ⬇️
pallets/creditcoin/src/migrations/v2.rs 92.30% <88.88%> (-2.55%) ⬇️
... and 13 more

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

atodorov and others added 9 commits January 17, 2023 13:08
@atodorov
Remove unused scripts/init.sh
e81ae42
@atodorov
Add pre_upgrade/ & post_upgrade hooks in v7
8c2ac87 
for use with try-runtime and upgrades testing
@atodorov
Add script which will assert on migrations
256d2d5
@atodorov
Add the missing post_upgrade() and StorageVersion assertions
ee6b75d 
which will satisfy check-storage-version-assert-in-migrations.sh
@atodorov
Redesign how pre_upgrade()/post_upgrade() functions are called
01612c1 
these are no longer Substrate hooks but rather a place where we can
perform more assertions while testing migrations in CI.

scripts/check-storage-version-assert-in-migrations.sh now becomes
useless.
@atodorov
Update migrate() to use a loop & a map of callbacks
38cc926 
makes the code more concise and easier to read and removes the explicit
checks for version number before applying the next migration!
@CAGS295 @atodorov
move difficulty primitive
02af4fd
@CAGS295 @atodorov
Migration trait
c01e6af
@atodorov
Bump version
ffee704
AdaJane
AdaJane previously approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@AdaJane AdaJane left a comment

Choose a reason for hiding this comment

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

This updated implementation seems like a big improvement.

CAGS295
CAGS295 previously approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@CAGS295 CAGS295 left a comment

Choose a reason for hiding this comment

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

Not sure if my review is valid but here you go.

nathanwhit
nathanwhit previously approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@nathanwhit nathanwhit left a comment

Choose a reason for hiding this comment

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

A bunch of nitpicks but nothing big or blocking

pallets/creditcoin/src/migrations/v1.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v2.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v2.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v3.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v2.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v3.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v4.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v4.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v7.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v7.rs Outdated Show resolved Hide resolved
@atodorov atodorov dismissed stale reviews from nathanwhit, CAGS295, and AdaJane via dcc774e January 18, 2023 16:27
@atodorov atodorov merged commit 2762f58 into dev Jan 18, 2023
@atodorov atodorov deleted the more-tests-for-CSUB-264 branch January 18, 2023 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanwhit nathanwhit nathanwhit approved these changes

@CAGS295 CAGS295 CAGS295 approved these changes

@AdaJane AdaJane Awaiting requested review from AdaJane

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@atodorov @codecov-commenter @nathanwhit @CAGS295 @AdaJane