test: add missing test cases for bids #159
Conversation
rabi-siddique
force-pushed
the
rs-additional-cli-tests
branch
2 times, most recently
from
210ae8a to
bbf89d3
Compare
|
The tests related to viewing bids are currently failing due to a known issue. This issue is being tracked in the following pull request: Agoric/agoric-sdk#9024. I will proceed with this PR once the related issue is resolved. |
|
The test that involves changing prices using the |
rabi-siddique
force-pushed
the
rs-additional-cli-tests
branch
from
bbf89d3 to
4ea85ba
Compare
| @@ -3,5 +3,5 @@ | |||
|
|
|||
| source ./test/e2e/test-scripts/common.sh | |||
|
|
|||
| agops inter bid by-discount --discount 5 --give 2IST --from $accountAddress --keyring-backend=test 2>&1 | |||
| agops inter bid by-discount --discount 5 --give 2IST --from $user2Address --keyring-backend=test 2>&1 | |||
There was a problem hiding this comment.
2>&1 doesn't seem appropriate here - inter bid sends data (in JSON) to stdout and misc. progress info to stderr.
Oh... but I don't see anything consuming any of the output. So maybe it doesn't matter.
There was a problem hiding this comment.
ah. I see the .js code consumes the output.
test/e2e/specs/test.spec.js
Outdated
| @@ -57,6 +57,15 @@ describe('Wallet App Test Cases', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('should view the bid from CLI', () => { | |||
| cy.exec('bash ./test/e2e/test-scripts/view-bids.sh', { | |||
There was a problem hiding this comment.
view-bids.sh is essentially a 1 liner. How about just inlining it into the .js code?
There was a problem hiding this comment.
Yes, let's do that. I need to combine some variables from common.sh with the command in view-bids.sh. Should we prioritize consistency since we're using .sh files in other shell-based tests?
There was a problem hiding this comment.
Also, I was running the test for view.bids.sh. And it tends to give the same issue solved here:
Agoric/agoric-sdk#9024
But if I run the agops inter bid list from the shell, all works as expected. I reckon we're some using old dependencies in the wallet-app?
There was a problem hiding this comment.
Should we prioritize consistency since we're using .sh files in other shell-based tests?
I prefer to review as little .sh code as possible.
There was a problem hiding this comment.
But if I run the agops inter bid list from the shell, all works as expected. I reckon we're some using old dependencies in the
wallet-app?
I'm still trying to figure out why we're testing the bidding CLI code from agoric-sdk in the wallet-app repo at all.
But yes, we seem to be getting agoric-sdk (including the bidding CLI) from a published docker image for agoric-sdk... I suppose that's from a recent release.
agoric-sdk:latest ... looks like it's currently 20240420012145-734e86 aka agoric-upgrade-15-rc0 Apr 20.
The fix, 9024 wasn't merged until Apr 22.
The instructions for bidding don't say to use code from such a release. They say to do:
git clone https://github.com/Agoric/agoric-sdk
cd agoric-sdk
SKIP_DOWNLOAD=false ./bin/agd build
export PATH=$PWD/bin:$PATH
alias inter="yarn run --silent agops inter"
export AGORIC_NET=main
Installing straight from master is a little goofy (long story... agops is a test tool but we're using it as an end-user tool... cc @otoole-brendan ), but until we decide to do otherwise, I suppose we should do that in ci.
There was a problem hiding this comment.
The bids placed from the CLI have an element of UI testing as well. Once a bid is placed from the CLI, it is visible on the wallet-app UI.
There was a problem hiding this comment.
Initially, I was installing directly from the master and it was goofy :D
It also involved setting node and Golang in CI. The CI test runs took more than 15 minutes till I found agoric-sdk image.
There was a problem hiding this comment.
I prefer to review as little
.shcode as possible.
I'm working on eliminating bash scripts from liquidation tests, in this pull request. I'm also planning to apply a similar approach to the bid-related tests
There was a problem hiding this comment.
Initially, I was installing directly from the master and it was goofy :D It also involved setting node and Golang in CI. The CI test runs took more than 15 minutes till I found
agoric-sdkimage.
Really? ci in the docs repo installs agoric-sdk from source and it runs in under 4min:
https://github.com/Agoric/documentation/actions/runs/8707389744/job/23882252835
I'm not sure what to tell you... if we want to test the product that we tell users to use for bidding, that's not the agoric-sdk image.
rabi-siddique
force-pushed
the
rs-additional-cli-tests
branch
from
0237bae to
9c70872
Compare
The PR adds tests for: