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

[pox-4] Pool Stacking stateful property-based tests #4550

Merged
merged 131 commits into from Apr 24, 2024

Conversation

moodmosaic
Copy link
Member

@moodmosaic moodmosaic commented Mar 16, 2024

Description

This is an integral part of our commitment to adopting a stateful property-based testing strategy for PoX-4.

Applicable issues

Additional info (benefits, drawbacks, caveats)

The stateful property-based tests can run via npm test and also directly via

npx vitest watch tests/pox-4/pox-4.stateful-prop.test.ts --reporter=dot

Sample output

✓ wallet_4        allow-contract-caller  wallet_3               until                  179942593
✓ wallet_8        delegate-stx           amount                 33784629952454         delegated to           wallet_2               until                  1875298417
✓ wallet_4        allow-contract-caller  wallet_4               until                  none
✓ wallet_9        delegate-stx           amount                 36970141191493         delegated to           wallet_9               until                  1405718799
✓ wallet_4        allow-contract-caller  wallet_7               until                  1739629828
✓ wallet_5        stx-account            lock-amount            625000000000           unlocked-amount        99375000000000         unlocked-height        11550
✓ wallet_9        revoke-delegate-stx
✓ wallet_5        get-stacking-minimum   pox-4                  125000000000
✓ wallet_9        allow-contract-caller  wallet_4               until                  858124873
✓ wallet_2        stx-account            lock-amount            1525922319735          unlocked-amount        98474077680265         unlocked-height        12600
✓ wallet_7        allow-contract-caller  wallet_1               until                  1307633494
✓ wallet_9        stx-account            lock-amount            625000000000           unlocked-amount        99375000000000         unlocked-height        13650
✓ wallet_7        revoke-delegate-stx
✓ wallet_9        delegate-stx           amount                 11433545341619         delegated to           wallet_2               until                  1653742613
✓ wallet_2        allow-contract-caller  wallet_8               until                  1376025588
✓ wallet_5        get-stacking-minimum   pox-4                  125000000000
✓ wallet_9        get-stacking-minimum   pox-4                  125000000000
✓ wallet_2        stx-account            lock-amount            1525922319735          unlocked-amount        98474077680265         unlocked-height        12600
✓ wallet_1        allow-contract-caller  wallet_7               until                  89399923
✓ wallet_6        delegate-stx           amount                 34970137606759         delegated to           wallet_2               until                  288872382
✓ wallet_9        get-stacking-minimum   pox-4                  125000000000

Copy link

codecov bot commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.97%. Comparing base (d0df9d1) to head (fe2777b).
Report is 61 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4550      +/-   ##
==========================================
- Coverage   83.36%   82.97%   -0.40%     
==========================================
  Files         470      470              
  Lines      332151   332768     +617     
  Branches      317      317              
==========================================
- Hits       276905   276100     -805     
- Misses      55238    56660    +1422     
  Partials        8        8              

see 77 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0df9d1...fe2777b. Read the comment docs.

@moodmosaic moodmosaic force-pushed the feat/pox-4-stateful-property-testing branch 4 times, most recently from b5814a4 to b86104d Compare March 18, 2024 00:28
@moodmosaic moodmosaic force-pushed the feat/pox-4-stateful-property-testing branch 2 times, most recently from fd29768 to 5d79ffb Compare March 26, 2024 11:17
@moodmosaic moodmosaic force-pushed the feat/pox-4-stateful-property-testing branch 2 times, most recently from 31f0901 to e21db00 Compare March 27, 2024 00:03
@moodmosaic moodmosaic force-pushed the feat/pox-4-stateful-property-testing branch from 3c085f8 to 5679afa Compare April 22, 2024 14:33
@moodmosaic moodmosaic requested review from setzeus and friedger and removed request for friedger April 22, 2024 15:41
@moodmosaic moodmosaic changed the base branch from next to develop April 23, 2024 08:43
Copy link
Collaborator

@friedger friedger left a comment

Choose a reason for hiding this comment

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

Should be against dev.

I reviewed the smaller PRs, so only quickly looked at the whole code again.

setzeus

This comment was marked as outdated.

@setzeus setzeus self-requested a review April 23, 2024 10:28
Copy link
Collaborator

@setzeus setzeus left a comment

Choose a reason for hiding this comment

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

Have seen the individual branches along the way, kudos to the progress, excited to get this merged in!

Copy link
Collaborator

@hugocaillard hugocaillard left a comment

Choose a reason for hiding this comment

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

Leaving a "request for change" so that it doesn't get merged while I investigate

it("statefully interacts with PoX-4", async () => {
// SUT stands for "System Under Test".
const sut: Real = {
network: await initSimnet(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be causing the issue identified by @BowTiedRadone
I'm currently investigating

Copy link
Member Author

Choose a reason for hiding this comment

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

@hugocaillard

  • Can we disable the implicit/global simnet object for this test?
    it("statefully interacts with PoX-4", async () => {
      // No implicit/global `simnet` object for the scope of this test.
    });
  • Or perhaps I can reference the implicit/global simnet instead of await initSimnet().

FWIW, in the long term I want these tests to run outside of Vitest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we disable the implicit/global simnet object for this test?

That would be doable by tweaking the setup file but I don't see a lot of advantage to do so.

Or perhaps I can reference the implicit/global simnet instead of await initSimnet().

That would make sense yes 👍 It wouldn't fix the issue though, it wouldn't have much of an impact but I still recommend it

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I can reference the implicit/global simnet instead of await initSimnet().

@hugocaillard
Copy link
Collaborator

@moodmosaic @BowTiedRadone
In the unit test PR #4698, we moved the unit tests to an other directory called boot-contracts-unit-tests. Would you consider using a similar name for these tests? (eg: boot-contracts-property-tests?)

@moodmosaic
Copy link
Member Author

Would you consider using a similar name for these tests? (eg: boot-contracts-property-tests?)

Yes. Sounds reasonable. 👍

@moodmosaic
Copy link
Member Author

@hugocaillard: @BowTiedRadone is working on a branch to add the remaining solo staking scenarios (this PR adds the pool stacking ones). Perhaps we can add #4550 (comment) and #4550 (comment) there and get this merged now (unless you consider those as blockers, but I think they are not).

Copy link
Collaborator

@hugocaillard hugocaillard left a comment

Choose a reason for hiding this comment

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

Sounds good @moodmosaic and great job @BowTiedRadone. It's amazing to look at all of these tests running 🎉

@moodmosaic moodmosaic merged commit 13081b2 into develop Apr 24, 2024
251 of 295 checks passed
@moodmosaic
Copy link
Member Author

amazing to look at all of these tests running 🎉

And we just started. Stay tuned 🚀

@moodmosaic
Copy link
Member Author

@hugocaillard

In the unit test PR #4698, we moved the unit tests to an other directory called boot-contracts-unit-tests. Would you consider using a similar name for these tests? (eg: boot-contracts-property-tests?)

Now that @BowTiedRadone opened #4725, we can do that (inside that PR or probably better as a separate one).


@hugocaillard, in #4698 the boot-contracts-unit-tests is a separate Vitest project with its own config files.

In our case, then, we are going to do the same with boot-contracts-stateful-prop-tests and have a separate Vitest project for it, or even better (in the context of stateful property testing) have it running standalone as a Node.js app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants