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

test(a3p): migrate wallet and vaults related tests from a3p-proposals to the z:acceptance #10123

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Jorge-Lopes
Copy link

@Jorge-Lopes Jorge-Lopes commented Sep 20, 2024

closes: Agoric/BytePitchPartnerEng#5
closes: Agoric/BytePitchPartnerEng#7
refs: #10049

Description

This PR is part of an ongoing effort to migrate some of the selected test cases from a3p-proposals to the z:acceptance test phase.

This particular PR new tests focus on:

  • invitations and payments behavior within the smart wallet
  • actions that can be executed on vaults

The source code of the migrated tests are:

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

One commit included in this PR fixes a small typo on a3p-integration/proposals/README.md

Testing Considerations

The new test files included in this PR are invoked through test.sh, so no changes to the current testing workflow are necessary.

However, as pointed out by @dckc in this comment, using waitForBlock() can lead to unexpected behavior. To address this, we plan to update the z:acceptance tests to mitigate this issue.

As a solution, we propose extending the @agoric/synthetic-chain package to export a method like makeRetryUntilCondition or a similar function. This would provide a more reliable alternative to waitForBlock().

Additionally, there is already an test-vaults.mts file that test operations related to changes on auctions parameters.
To avoid confusion with vaults.test due to the similarity in names, I propose renaming this file to auction.test and expanding its scope to cover all auction-related tests.

Upgrade Considerations

n/a

@Jorge-Lopes Jorge-Lopes changed the title Jorge/acceptance migration wallet test(a3p): migrate wallet and vaults related tests from a3p-proposals to the z:acceptance Sep 20, 2024
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

I'd like to see a list of what test in a3p these replace.

a3p-integration/proposals/z:acceptance/test.sh Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/exitOffer.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/vaults.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/vaults.test.js Outdated Show resolved Hide resolved
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

IOU more info about which of these comments is critical vs. just suggestions.

a3p-integration/proposals/z:acceptance/vaults.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/vaults.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/vaults.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/vaults.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/wallet.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/wallet.test.js Outdated Show resolved Hide resolved
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 24, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 24, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 24, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 24, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 24, 2024
@Jorge-Lopes
Copy link
Author

I'd like to see a list of what test in a3p these replace.

I updated the PR's description to provide the list of replaced a3p tests, being them:

Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 25, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 25, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 25, 2024
a3p-integration/proposals/z:acceptance/test-lib/utils.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/test-lib/utils.js Outdated Show resolved Hide resolved
Comment on lines 25 to 34
const error = await t.throwsAsync(() =>
openVault(USER1ADDR, mint, collateral),
);

t.true(
error?.message.includes(
'Error: Vault creation requires a minInitialDebt of {"brand":"[Alleged: IST brand]","value":"[5000000n]"}',
),
'Error message does not contain the expected text',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

The form I'm more used to seeing is this

Suggested change
const error = await t.throwsAsync(() =>
openVault(USER1ADDR, mint, collateral),
);
t.true(
error?.message.includes(
'Error: Vault creation requires a minInitialDebt of {"brand":"[Alleged: IST brand]","value":"[5000000n]"}',
),
'Error message does not contain the expected text',
);
await t.throwsAsync(
openVault(USER1ADDR, mint, collateral),
message: 'Error: Vault creation requires a minInitialDebt of {"brand":"[Alleged: IST brand]","value":"[5000000n]"}',
);

There are forms that use regexps with // pattterns, as well.

Copy link
Author

Choose a reason for hiding this comment

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

I missed the earlier suggestion regarding the use of regular expressions made by @dckc on this comment for the error message. I wasn't aware that a regular expression could be provided as the expectation in t.throwsAsync(), which is why I overlooked it initially.

I also apologize for prematurely resolving the conversation. I understand that it made it harder to track the changes made and open questions. Moving forward, I will make sure to leave conversations open and wait for the original commenter to resolve them.

Thank you for your patience and guidance.

Changes addressed on commit 2f7d52d

a3p-integration/proposals/z:acceptance/vaults.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/vaults.test.js Outdated Show resolved Hide resolved
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 27, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 27, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 27, 2024
@Jorge-Lopes Jorge-Lopes requested a review from a team as a code owner September 27, 2024 13:44
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 27, 2024
Jorge-Lopes added a commit to bytepitch/agoric-sdk that referenced this pull request Sep 27, 2024
@Jorge-Lopes Jorge-Lopes force-pushed the jorge/acceptance-migration-wallet branch from 4216ee1 to 0b1501a Compare September 30, 2024 10:51
@Jorge-Lopes Jorge-Lopes reopened this Sep 30, 2024
@Jorge-Lopes
Copy link
Author

I noticed that the original pull request included merge commits due to fetching the state of the upstream repository.
To align with the guidelines to to keep the commit history clean, I have rebased my branch against the upstream master branch and removed the unnecessary merge commits.
This process resulted in the PR being closed and reopened.

I am sorry for any inconvenience and please let me know if anything else is needed.
Thank you.

@Chris-Hibbert
Copy link
Contributor

We don't usually require a re-review after rebasing for merge. Did anything interesting change? If not, my previous approval holds.

@LuqiPan LuqiPan added the automerge:rebase Automatically rebase updates, then merge label Oct 2, 2024
@Chris-Hibbert
Copy link
Contributor

@LuqiPan pointed out that the merge process may require more assistance from us. Sorry, I didn't realize that. We (Luqi) are looking into that.

@Jorge-Lopes
Copy link
Author

@LuqiPan pointed out that the merge process may require more assistance from us. Sorry, I didn't realize that. We (Luqi) are looking into that.

Thank you for the feedback.
If there's anything else we can do to help with the merge process, just let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate existing vaults related tests to z:acceptance migrate existing wallet related tests to z:acceptance
4 participants