-
Notifications
You must be signed in to change notification settings - Fork 32
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
Introduce E2E tests for the faucet bot and server #198
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me.
But seriously, good pr. Lots of setups I see.
Should we also add tests about the bot interacting with invalid addresses?
Yes, I would be adding some more test cases in a separate PR. |
…nned patterns in history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit about reusing common code, still going through test setup configuration (upd: test setup configuration LGTM)
src/utils.ts
Outdated
await sleep(step); | ||
} | ||
throw new Error(`waitUntil timed out after ${timeout} ms`); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already delay
and and until
functions in opstooling-js
.
until
from opstooling-js
also has timeoutMessage
parameter, which would be helpful in tests, to point at expectation that did not occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out - applied.
src/utils.ts
Outdated
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
type Predicate = () => any | (() => Promise<any>); | ||
|
||
export const waitUntil = async (predicate: Predicate, step = 100, timeout = 10000) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the timeout here's the same as the governing timeout for the tests, meaning that this timeout condition will never fire, and instead we'll be getting timeouts from jest, without stacktrace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, timeouts adjusted.
IMO setting up docker containers using docker exec and wait-for in bash scripts can be flaky. I would recommend using a proper docker test framework such as https://github.com/testcontainers/testcontainers-node |
Good point actually. This advice could save me quite some time a month ago :) |
I had no idea this existed, I will check it out. |
That looks super useful! Will keep it in mind |
e2e/bootstrap.sh
Outdated
# Start Polkadot and Matrix and wait until they are up. | ||
docker-compose up -d | ||
source wait_until.sh 'curl -s "localhost:8008"' | ||
source wait_until.sh 'curl -s "localhost:9933"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should technically be 127.0.0.1 instead of localhost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, updated.
First, we spin up:
//Alice
seed so it is fundedNext, we test the behaviour by sending
!balance
and!drip
messages.