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

Remove encoding/decoding and update tests #35

Merged
merged 16 commits into from
Dec 24, 2024
Merged

Conversation

rabi-siddique
Copy link
Collaborator

@rabi-siddique rabi-siddique commented Dec 12, 2024

refs: #30

The PR does the following:

  • It updates the tests that were failing. Updated the wallet test to use .current and corrected the Date object. For the state change test, replaced the initial payload (no vault close event found at 627) with one for a vault opened at block height 627.
  • Updated the image reference to ghcr.io/agoric/agoric-3-proposals@sha256:644e09bf041e5588570ff9a49e6129cc468e9c8458417541027e8532cb91083b instead of latest to ensure the Date object remains deterministic in tests. @frazarshad and I observed that the recent a3p image, published 3 days ago, caused block dates in the tests to reflect that publish date.
  • Upgraded the subql-node-cosmos image to version 4.2.1, where handleStateChangeEvent handles events without encoding, allowing us to remove the encoding/decoding sections from the code.

@rabi-siddique rabi-siddique changed the base branch from main to ta/ci-test December 12, 2024 12:22
@rabi-siddique rabi-siddique force-pushed the rs-test branch 7 times, most recently from aea3527 to 3dc20cf Compare December 13, 2024 11:23
@rabi-siddique rabi-siddique force-pushed the rs-test branch 2 times, most recently from db9b2eb to 57a190f Compare December 13, 2024 13:10
@rabi-siddique rabi-siddique changed the title testing Remove encoding/decoding and update tests Dec 13, 2024
@@ -7,7 +7,7 @@ jobs:
runs-on: ubuntu-latest
services:
a3p:
image: ghcr.io/agoric/agoric-3-proposals:latest
image: ghcr.io/agoric/agoric-3-proposals@sha256:644e09bf041e5588570ff9a49e6129cc468e9c8458417541027e8532cb91083b
Copy link
Member

Choose a reason for hiding this comment

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

Ideally it works with latest but we don't have CI set up yet to trigger here when latest changes. Meanwhile it should at least use a tag.

Does use-upgrade-17 work? That's the current latest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using the use-upgrade-17 tag worked. Updated in #36

package.json Outdated
@@ -25,7 +25,7 @@
"devDependencies": {
"@cosmjs/stargate": "^0.28.9",
"@subql/cli": "^5.4.0",
"@subql/node-cosmos": "^4.2.1",
"@subql/node-cosmos": "4.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

please retain semver range. the lockfile controls whether the update actually changes

Heads up that a new release is imminent with this dependency updates (https://github.com/subquery/subql-cosmos/pull/302) fixing the exit code for the test runner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. Updated in #36

[
new Wallet(
'published.wallet.agoric1ee9hr0jyrxhy999y755mp862ljgycmwyp4pl7q',
'published.wallet.agoric1rwwley550k9mmk6uq6mm6z4udrg8kyuyvfszjk.current',
Copy link
Member

Choose a reason for hiding this comment

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

looking at .current is more comprehensive but a path without .current should also work.

vaultManagerMetricsDaily.totalShortfallReceivedLast = BigInt(0);
vaultManagerMetricsDaily.metricsCount = BigInt(1);

// DISABLED because the timestamp for these entities is changed on each run of the a3p container
Copy link
Member

Choose a reason for hiding this comment

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

are you saying the time for the block (e.g. 1212) changes with each run?

any ideas why these paths have that problem and the ones below like boardAux don't?

I looked into expressing the tests to not expect a certain date but the subqlTest runner expects a full exact match: https://github.com/subquery/subql/blob/cb2db57fa3be7a3e4033cc170ea5d8f45fadf90c/packages/node-core/src/indexer/test.runner.ts#L96-L107

thankfully we can instead automate testing using GraphQL queries as you've done in #36

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes. it seems the blocks that contain the data for vault metrics are committed after the chain is started, therefore the time the block is committed is different on each time the a3p container is started.

#36 seems like a good approach for this. i'll see what i can do 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made the update in #38

@rabi-siddique rabi-siddique merged commit e92d16b into ta/ci-test Dec 24, 2024
2 checks passed
@rabi-siddique rabi-siddique deleted the rs-test branch December 24, 2024 07:20
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.

3 participants