test(a3p): extend test coverage for KREAd app on z:acceptance test phase #10242

Jorge-Lopes · 2024-10-08T17:40:48Z

closes: https://github.com/Agoric/BytePitchPartnerEng/issues/12
refs: https://github.com/Agoric/BytePitchPartnerEng/issues/9

Description

The existing test of the KREAd application on the z:acceptance test phase was limited to the feature allowing users to mint a new KREAd Character.

This PR extends the existing test coverage to include the following cases:

minting a new Character
unequip all defaults Items
sell unequipped Item
buy an Item on marketplace
sell a Character
buy a Character on marketplace

Along with the kread.test.js file, this PR includes a new file, /test-lib/kread.js, that provides all the required helper functions for the test cases above, allowing a cleaner and scalable design for KREAd tests.

Since the test coverage of the new file overlaps with the existing one, I opted for removing both:

create-kread-item-test.sh
generate-kread-item-request.mjs

Security Considerations

No security considerations were identified.

Scaling Considerations

No scaling considerations were identified.

Documentation Considerations

No documentation considerations were identified.

Testing Considerations

I have confirmed that this works fine with existing a3p tests locally, there is still an improvement that may be useful to ensure that the user wallet's purse balance for the desired KREAd asset was updated accordingly.

For this purpose, we intend to use one feature included in the currently open PR #10171 , which is the sync-tools method retryUntilCondition().

However, we hope to address those changes in a different PR that will be the result of the effort put into the currently open ticket

cloudflare-workers-and-pages · 2024-10-08T17:43:04Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`f427d35`
Status:	✅ Deploy successful!
Preview URL:	https://64c39d45.agoric-sdk.pages.dev
Branch Preview URL:	https://jorge-acceptance-kread-test.agoric-sdk.pages.dev

View logs

Chris-Hibbert

Looks pretty good. I'd like to see an additional helper, and more use of Amounts.

a3p-integration/proposals/z:acceptance/test-lib/kread.js

Chris-Hibbert · 2024-10-18T23:45:37Z

a3p-integration/proposals/z:acceptance/test-lib/kread.js

+  const path = `:published.kread.market-characters.${charactersMarket[0]}`;
+  const rawCharacterData = await agoric.follow('-lF', path, '-o', 'text');
+  const marketCharacter = marshaller.fromCapData(JSON.parse(rawCharacterData));


please write a helper that will unmarshall from vstorage to Amounts.

In the effort to keep the new helper function getAssetAmount agnostic to the asset provided as an argument, I left the query to Vstorage unchanged and used marketCharacter as the argument.
Does this approach work for you?

I'm fine with passing marketCharacter to totalAssetPriceAmount, but I'd still prefer that there be a function for the 4 lines above. (perhaps getMarketCharacterFromVstorage()).
is 0 a parameter saying which child, or is it always ':published.kread.market-characters.${charactersMarket[0]}'?

I addressed the requested change by implementing the new getMarketCharacterFromVstorage helper function.

In this case, there is no need to provide the market-characters children node dynamically because, for the test case built, we only need to ensure that a character is available on the market.
For that reason, I am using the static child index 0.

a3p-integration/proposals/z:acceptance/kread.test.js

a3p-integration/proposals/z:acceptance/test-lib/kread.js

Chris-Hibbert · 2024-10-23T17:29:46Z

a3p-integration/proposals/z:acceptance/test-lib/kread.js

+  const assetValue = makeCopyBag([[asset, 1n]]);
+  const assetAmount = AmountMath.make(
+    brand,
+    //@ts-expect-error casting


This shouldn't be necessary. AmountMath.make takes CopyBagAmounts by default, so you should be able to declare a type for assetValue and not have to expect an error. The following worked for me

/** @import {Brand, CopyBagAmount} from '@agoric/ertp'; */ /** * @param {Brand} brand * @param {any} asset * @returns {CopyBagAmount<'item'>} */ export const getAssetAmount = (brand, asset) => { const assetValue = makeCopyBag([[asset, 1n]]); return AmountMath.make(brand, assetValue); };

Requested change addressed

Jorge-Lopes · 2024-10-24T13:20:17Z

@Chris-Hibbert I am sorry for having requested your re-review yesterday before noticing the linting issues.

Chris-Hibbert

This is on the right track. Just a few requests, not all required.

Chris-Hibbert · 2024-10-24T22:19:29Z

a3p-integration/proposals/z:acceptance/test-lib/kread.js

+  const path = `:published.kread.market-characters.${charactersMarket[0]}`;
+  const rawCharacterData = await agoric.follow('-lF', path, '-o', 'text');
+  const marketCharacter = marshaller.fromCapData(JSON.parse(rawCharacterData));


I'm fine with passing marketCharacter to totalAssetPriceAmount, but I'd still prefer that there be a function for the 4 lines above. (perhaps getMarketCharacterFromVstorage()).
is 0 a parameter saying which child, or is it always ':published.kread.market-characters.${charactersMarket[0]}'?

a3p-integration/proposals/z:acceptance/kread.test.js

a3p-integration/proposals/z:acceptance/test-lib/kread.js

Chris-Hibbert · 2024-10-24T22:54:07Z

a3p-integration/proposals/z:acceptance/test-lib/kread.js

+    kreadCharacter2,
+  );
+
+  const id = `KREAd-unequip-all-items-acceptance-test`;


just a suggestion: I give a separate name to values like this if I'm going to use them more than once, or if pulling them out will make the formatting take fewer lines. Why do you prefer this over

id: `KREAd-unequip-all-items-acceptance-test`,

inside offer?

Thank you for the suggestion—I've made the change.

I don't have a strong preference; it was simply a habit of mine.
I now understand your reasoning for when to declare the offer ID outside of the offer body.
I’ll adopt that approach moving forward.

Jorge-Lopes · 2024-10-28T11:08:35Z

I'm fine with passing marketCharacter to totalAssetPriceAmount, but I'd still prefer that there be a function for the 4 lines above. (perhaps getMarketCharacterFromVstorage()).
is 0 a parameter saying which child, or is it always ':published.kread.market-characters.${charactersMarket[0]}'?

Change requested addressed by implementing the getMarketCharacterFromVstorage helper function.

In this case, there is no need to provide the market-characters children node dynamically because, for the test case built, we only need to ensure that a character is available on the market. For that reason, I am using the static the child index 0.

a3p-integration/proposals/n:upgrade-next/agoric-tools.js

a3p-integration/proposals/n:upgrade-next/test.sh

a3p-integration/proposals/n:upgrade-next/use.sh

a3p-integration/proposals/z:acceptance/governance.test.js

a3p-integration/proposals/z:acceptance/package.json

Jorge-Lopes · 2024-10-28T20:07:50Z

@Chris-Hibbert while addressing the conflicts in order to rebase this branch into master, I mistakenly included commits that were not done at this branch.
I am currently trying to clean the commit history.

Meaning that most of the comments you just raised regards code the I did not do it.
Regarding the agoric packages versions, I will change to "dev" as you suggested.

github-actions · 2024-10-29T09:43:02Z

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

github-actions · 2024-10-29T11:02:55Z

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

Jorge-Lopes · 2024-10-29T12:25:43Z

Greetings @Chris-Hibbert

As I mentioned before, while addressing the conflicts after rebasing to master, the commit history displayed unexpected behavior. It showed duplicated commits as well as commits that had already been merged on the master.

While trying different approaches to solving the issue, one suggested editing the landing branch to a different one and then back to the original, which is why you can see that change in the conversation and the reason why I added the 'force:integration' label to re-run the integration tests.

I was able to solve the problems by doing an interactive rebase, having now a clean commit history, and maintaining the changes that you requested during this review.

Thank you for your time and sorry for any inconvenience.

Chris-Hibbert

LGTM.

please add one comment before merging.

a3p-integration/proposals/z:acceptance/test-lib/kread.js

rel: Agoric/BytePitchPartnerEng#13

Co-authored-by: Chris Hibbert <Chris-Hibbert@users.noreply.github.com>

rel: Agoric/BytePitchPartnerEng#13

Jorge-Lopes requested a review from a team as a code owner October 8, 2024 17:40

Jorge-Lopes requested a review from AgoricTriage October 8, 2024 17:40

Jorge-Lopes force-pushed the jorge/acceptance-kread-test branch 2 times, most recently from 55f9a44 to a05465b Compare October 11, 2024 18:08

Chris-Hibbert requested changes Oct 19, 2024

View reviewed changes

Jorge-Lopes force-pushed the jorge/acceptance-kread-test branch from ae08130 to 91edc18 Compare October 23, 2024 11:23

Jorge-Lopes requested a review from Chris-Hibbert October 23, 2024 17:08

Chris-Hibbert requested changes Oct 23, 2024

View reviewed changes

Jorge-Lopes force-pushed the jorge/acceptance-kread-test branch from ea0db37 to 9082311 Compare October 24, 2024 12:00

Jorge-Lopes requested a review from Chris-Hibbert October 24, 2024 13:14

Chris-Hibbert reviewed Oct 24, 2024

View reviewed changes

Jorge-Lopes requested a review from Chris-Hibbert October 28, 2024 12:12

Chris-Hibbert reviewed Oct 28, 2024

View reviewed changes

Jorge-Lopes changed the base branch from master to jorge/verify-pushed-price October 29, 2024 09:42

Jorge-Lopes changed the base branch from jorge/verify-pushed-price to master October 29, 2024 09:42

Jorge-Lopes changed the base branch from master to jorge/acceptance-wallet-test-sync-tools October 29, 2024 11:02

Jorge-Lopes changed the base branch from jorge/acceptance-wallet-test-sync-tools to master October 29, 2024 11:02

Jorge-Lopes force-pushed the jorge/acceptance-kread-test branch from f6481e1 to 5645723 Compare October 29, 2024 11:25

Jorge-Lopes added the force:integration Force integration tests to run on PR label Oct 29, 2024

Jorge-Lopes requested a review from Chris-Hibbert October 29, 2024 12:26

Chris-Hibbert approved these changes Oct 30, 2024

View reviewed changes

a3p-integration/proposals/z:acceptance/test-lib/kread.js Show resolved Hide resolved

Jorge-Lopes added 2 commits October 31, 2024 09:34

test(a3p): add test cases for the KREAd app

9fb4115

rel: Agoric/BytePitchPartnerEng#13

chore(a3p): add helper functions for KREAd tests

f15bfc8

rel: Agoric/BytePitchPartnerEng#13

Jorge-Lopes and others added 10 commits October 31, 2024 09:34

chore(a3p): replace old test scripts due to duplicated coverage

d9c50da

rel: Agoric/BytePitchPartnerEng#13

chore(a3p): add helper methods and remove unnecessary comments

4f29074

rel: Agoric/BytePitchPartnerEng#13

fix(a3p): lint fix

d61e5bb

fix(a3p):rename helper functions and provide type declaration

5e0a293

rel: Agoric/BytePitchPartnerEng#13

chore(a3p): change ertp version

76d36a2

test(a3p): add getMarketCharacterFromVstorage helper function

0f66f92

rel: Agoric/BytePitchPartnerEng#13

fix(a3p): fix typo

8872169

Co-authored-by: Chris Hibbert <Chris-Hibbert@users.noreply.github.com>

chore(a3p): move declare offer id inside its body

15362d4

rel: Agoric/BytePitchPartnerEng#13

chore(a3p): add @agoric/store: dev

10b5a5a

chore(a3p): provide in-line comment for unequipAllItemsOffer

f427d35

rel: Agoric/BytePitchPartnerEng#13

Jorge-Lopes force-pushed the jorge/acceptance-kread-test branch from 6431c66 to f427d35 Compare October 31, 2024 09:35

Jorge-Lopes added automerge:rebase Automatically rebase updates, then merge and removed force:integration Force integration tests to run on PR labels Oct 31, 2024

mergify bot merged commit fae2710 into master Oct 31, 2024
89 of 97 checks passed

mergify bot deleted the jorge/acceptance-kread-test branch October 31, 2024 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(a3p): extend test coverage for KREAd app on z:acceptance test phase #10242

test(a3p): extend test coverage for KREAd app on z:acceptance test phase #10242

Jorge-Lopes commented Oct 8, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 8, 2024 •

edited

Loading

Chris-Hibbert left a comment

Chris-Hibbert Oct 18, 2024

Jorge-Lopes Oct 23, 2024

Chris-Hibbert Oct 24, 2024

Jorge-Lopes Oct 28, 2024

Chris-Hibbert Oct 23, 2024

Jorge-Lopes Oct 24, 2024

Jorge-Lopes commented Oct 24, 2024

Chris-Hibbert left a comment

Chris-Hibbert Oct 24, 2024

Chris-Hibbert Oct 24, 2024

Jorge-Lopes Oct 28, 2024

Jorge-Lopes commented Oct 28, 2024

Jorge-Lopes commented Oct 28, 2024

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

Jorge-Lopes commented Oct 29, 2024

Chris-Hibbert left a comment

test(a3p): extend test coverage for KREAd app on z:acceptance test phase #10242

test(a3p): extend test coverage for KREAd app on z:acceptance test phase #10242

Conversation

Jorge-Lopes commented Oct 8, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

cloudflare-workers-and-pages bot commented Oct 8, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorge-Lopes commented Oct 24, 2024

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jorge-Lopes commented Oct 28, 2024

Jorge-Lopes commented Oct 28, 2024

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

Jorge-Lopes commented Oct 29, 2024

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Jorge-Lopes commented Oct 8, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 8, 2024 •

edited

Loading