-
Notifications
You must be signed in to change notification settings - Fork 61
E2E tests #640
base: development
Are you sure you want to change the base?
E2E tests #640
Conversation
✅ Deploy Preview for ubiquibot-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This is cool. Not mocked but connecting to an actual staging github application using a personal access token and verifying the results. At some point would be useful to add coverage output. |
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.
I can see @whilefoo you did a lot of works to setup unit tests + e2e.
but I feel like its a little bit away from the original goal.
In terms of unit tests, we should have coverage % in branch and function.
In terms of e2e, need to create a scenario and verify each step -- those need to be in workflow.
I am gonna contribute to this task as the codeowner because its an very important task for now and future. To contribute the task(To be honest, I wanna say this is far away from the completion),
What do you think @pavlovcik @rndquu @whilefoo ? If you agree about my contribution plan, please react with 👍 . |
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.
Just to clear the terminology, by "e2e testing" we mean running tests against a staging organization in a real github environment.
The original issue states the test script should file a new issue on a locally configurable repository (.env) that tests all the bot commands
. Covering all cases is a huge task so this PR tries to cover at least some cases.
I actually have hard time running the tests getting thrown: "Exceeded timeout of 10000 ms for a hook
error all the time:
commmands test
✕ /wallet correct address (5 ms)
✕ /wallet wrong address
✕ /multiplier
✕ /query
✕ /query wrong username
✕ /help
✕ /allow
✕ /start and /stop
● commmands test › /wallet correct address
thrown: "Exceeded timeout of 10000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."
150 | let issue: Issue;
151 |
> 152 | beforeAll(async () => {
| ^
153 | const res = await octokitAdmin.rest.issues.create({
154 | repo,
155 | owner,
at beforeAll (../tests/commands.test.ts:152:3)
at Object.<anonymous> (../tests/commands.test.ts:149:9)
Nevertheless I think that the current PR is good to merge and we should run e2e tests introduced in the current PR in a github CI ASAP because every new feature makes the e2e tests implemented in the current PR a bit obsolete.
I would:
- Merge the current PR
- Add a new CI workflow that runs e2e tests in a staging org on every push to a newly created PR by a bounty hunter (+fix the timeout errors)
- Create a new issue to refactor e2e tests step by step to cover all basic cases (i.e. commands)
- Start implementing unit tests (which is a totally different epic)
P.S. E2E tests on a staging org are gonna be really brittle but it is better than nothing
If the task is a minimal goal of overall E2E, we can merge the PR. but we should add From @rndquu 's comment, |
So we keep the current e2e tests and add a new "test workflow" as a part of the current issue, right? |
Right. In that way, we can make the PR valuable after the merge. |
I got rate limited one time during development, so I'm worried if we enable this for every commit, we will get rate limited quickly. |
Different GitHub CI events to consider, including only running on merge to |
Yes, perhaps we could run e2e tests only on push to the |
The problem I am seeing if we enable e2e on push event in the Even if we have rate limit, we should allow e2e tests on every pull requests to |
Yes. Perhaps we could add the |
Great idea! Any scope creep (this) can be spun off into a separate task though, so that we can merge asap? |
We have six hours to run tests. I don't see how timeout is an issue. Just increase the timeout time? |
I will fix the tests because there were some changes in between, make timeout 6 hours and add workflow |
It seems you can only manually run workflows on a branch and not PR |
As far as I understand the plan for the current PR is to fix current e2e tests and add a workflow that runs them |
Let's keep it simple, I think it's enough (for start) to have an e2e workflow with the |
Hey @whilefoo its been over a month. Why don't we leave it with a simple implementation so that we can merge asap. |
Successful workflow run: https://github.com/ubiquity-whilefoo/ubiquibot/actions/runs/6290628334 @0xcodercrane if you don't have any comments, let's merge this |
Next we should definitely test every incentive cause those are the most important features. |
Why do some take so long to run? Like start and stop taking 98 seconds? |
LOG_ENVIRONMENT: "production" | ||
SUPABASE_URL: ${{ secrets.SUPABASE_URL }} | ||
SUPABASE_KEY: ${{ secrets.SUPABASE_KEY }} | ||
X25519_PRIVATE_KEY: "QCDb30UHUkwJAGhLWC-R2N0PiEbd4vQY6qH2Wloybyo" |
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.
Should this be exposed? Not sure exactly what it is used for.
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.
it's okay, it must be the same because the test's config wallet private key in encrypted with this private key
It's a long test. Also there is some delay between because Github sometimes doesn't include latest comments right away so sometimes even with the delay it fails |
Resolves #633
Example of E2E test: ubiquibot-whilefoo-testing/ubiquibot#24