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: end-to-end bidding process testing #148

Merged
merged 11 commits into from
Apr 18, 2024

Conversation

rabi-siddique
Copy link
Contributor

@rabi-siddique rabi-siddique commented Mar 29, 2024

The PR does the following:

  • Add test cases for placing bids from the CLI.
  • Verify the existence of placed bids from the Wallet UI.
  • Cancelling bids from the Wallet UI.
  • Changes to ensure the CI pipeline doesn't break.

@rabi-siddique rabi-siddique changed the title test: e2e tests for placing bids test: end-to-end bidding process testing Mar 29, 2024
@rabi-siddique rabi-siddique force-pushed the rs-bidding-test-cases branch from bd31730 to 5ad00b6 Compare March 30, 2024 05:47
@rabi-siddique rabi-siddique force-pushed the rs-bidding-test-cases branch 3 times, most recently from b5ba2e0 to cfbfc44 Compare March 30, 2024 11:32
Dockerfile Outdated
ENV GOPATH="/go"
ENV GOBIN="/go/bin"

# Agoric SDK Setup
Copy link
Member

Choose a reason for hiding this comment

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

why is this building an SDK image? I'd expect it to use https://github.com/Agoric/agoric-sdk/pkgs/container/agoric-sdk ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware of it. Using the SDK image has improved the CI time. It's been halved now. Thanks for sharing!

But bidding-specific test cases are failing now for some reason. I am inspecting the issue and will get back to you once done with finding the root cause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@turadg, I was investigating the issue and discovered that it's related to a discrepancy in node versions.
The agoric-sdk image is configured with node 18.20and presently, our tests run smoothly with node version 18.18. I was able to replicate the failing test cases in our CI environment by upgrading my local node version to 18.20.

It's more so of an issue with cypress because inside the container I was successfully able to place bids from the shell.
I was trying to update cypress version for @agoric/synpress and make it compatible with node 18.20. But I feel it needs to be done carefully to avoid breaking other things.

Do you have any suggestions? Can we override the node version in agoric-sdk image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@turadg Finally working. I've used nvm to install node 18.18. When I run tests, I switch to node 18.18 with nvm use and it works.

Copy link
Member

@turadg turadg Apr 6, 2024

Choose a reason for hiding this comment

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

ok, if the tests require 18.18 to run please,

  • add a .node-version file that some Node version managers will observe automatically (I use nodenv which does)
  • file an issue on the repo to remove that restriction (it may just require updating ses and @endo` deps to latest)
  • point to that issue in all code used to pin the Node version

But it would be much better for @agoric/synpress to be compatible with 18.20. What is failing? I don't see anything in the Cypress repro about that Node version incompatibility. Some earlier version of ses failed on a change in 18.19. Maybe the problems can be solved by bumping other deps.

Let's figure out what part of the stack is broken in 18.20 before hacking so much around it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@turadg I think ses version is not an issue. I tried different versions and it gave no issues until I switched to node 18.20.

I also tried bumping versions of cypress and typescript. But they work fine till I use node 18.20. The issue doesn't exist for node versions up to 18.18. For versions beyond 18.18, excluding 20.4, the majority exhibit this issue.

The issue is the following error when trying to place bids with the agops command:

Removing intrinsics.Symbol.dispose
failed to delete intrinsics.Symbol.dispose (TypeError#1)
TypeError#1: Cannot delete property 'dispose' of function Symbol() { [native code] }

(TypeError#1)

I came across it over here as well:
Agoric/agoric-sdk#8599

package.json Outdated Show resolved Hide resolved
test/e2e/specs/test.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/test.spec.js Outdated Show resolved Hide resolved
@rabi-siddique rabi-siddique force-pushed the rs-bidding-test-cases branch 13 times, most recently from 1733b39 to e78601c Compare April 6, 2024 07:36
@rabi-siddique rabi-siddique force-pushed the rs-bidding-test-cases branch 4 times, most recently from 7cd2967 to 6a1fbd3 Compare April 6, 2024 11:25
Dockerfile Outdated Show resolved Hide resolved
@rabi-siddique rabi-siddique force-pushed the rs-bidding-test-cases branch from 6a1fbd3 to 4a76739 Compare April 16, 2024 04:10
@rabi-siddique rabi-siddique enabled auto-merge April 17, 2024 12:52
@rabi-siddique rabi-siddique disabled auto-merge April 17, 2024 16:52
@rabi-siddique rabi-siddique requested a review from turadg April 18, 2024 01:37
@rabi-siddique rabi-siddique force-pushed the rs-bidding-test-cases branch from 4a76739 to 39a742a Compare April 18, 2024 02:05
@rabi-siddique rabi-siddique force-pushed the rs-bidding-test-cases branch from 39a742a to 6bfa837 Compare April 18, 2024 03:22
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Thanks for the diligence in chasing the SES / Node error

@@ -0,0 +1,17 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

consider using Node.js scripts instead. Especially for tests. LTS has a built-in test runner https://nodejs.org/docs/latest-v20.x/api/test.html. Shell commands can run with https://nodejs.org/docs/latest-v20.x/api/child_process.html#child_processexecsynccommand-options. It even has JSON parsing ;-)

@rabi-siddique
Copy link
Contributor Author

Thanks for the diligence in chasing the SES / Node error

@turadg Thanks for pushing me to find the root cause of the problem :)

@rabi-siddique rabi-siddique merged commit a684537 into Agoric:main Apr 18, 2024
2 checks passed
@rabi-siddique rabi-siddique linked an issue Apr 19, 2024 that may be closed by this pull request
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.

Bidding E2E Testing Completed
2 participants