polish(auth): Better readme for testing commands#18372
Conversation
vpomerleau
left a comment
vpomerleau
left a comment
There was a problem hiding this comment.
Thanks for updating this documentation!
| ## Testing | ||
|
|
||
| Run tests with: | ||
| Before running tests make sure the correct services are running. If auth server is running in pm2 weird behavior can ensue, so |
There was a problem hiding this comment.
| Before running tests make sure the correct services are running. If auth server is running in pm2 weird behavior can ensue, so | |
| If auth server is running in pm2 weird behavior can ensue, so |
There was a problem hiding this comment.
For unit-tests, we actually want none of the services to be running?
There was a problem hiding this comment.
Unit tests do not need anything to be running. This is one of the main things that separates them from integration tests. If you look at our CI, unit tests can execute in total isolation. Now, should nothing be running at all? TBD… but it probably shouldn’t matter.
However, there's also this pre-existing comment in the readme that said “Note: stop the auth-server before running tests. Otherwise, they will fail with obscure errors.” This was written before we made a distinction between unit and integration tests... So not sure if it matters for unit-tests. I'd have to really vet this to be sure. I know for a fact a running auth-server messes with our test/remote tests.
Aside from quirks with auth server testing, in general, if you are working on unit tests I think it's good to not have ancillary things running. Do so and you might just be writing an integration test and not realizing it. That's why I think it's good practice to just run them after doing a yarn stop... ymmv.
| NODE_ENV=dev npx mocha -r esbuild-register test/*/** -g "SQSReceiver" | ||
| ``` | ||
| - `yarn start infrastructure` | ||
| - `nx start fxa-customs-server` |
There was a problem hiding this comment.
Maybe this would be a good spot to explain why customs is needed?
There was a problem hiding this comment.
I thought about this, but people can look at the code. And the reason may change overtime. Customs is an auxiliary service. I actually feel like customs deserves its own auth server specific section.
|
|
||
| - Note: stop the auth-server before running tests. Otherwise, they will fail with obscure errors. | ||
| - You can use `LOG_LEVEL`, such as `LOG_LEVEL=debug` to specify the test logging level. | ||
| - For quick enviroment config, consider running tests with a .env file and the dotenv command. For example: `dotenv -- yarn workspace fxa-auth-server:test-integration remote` |
There was a problem hiding this comment.
Should we note more specifically which variables are required?
There was a problem hiding this comment.
He does mention below: recovery-phone tests require twilio testing credentials!
I think those might be the only ones?
LZoog
left a comment
LZoog
left a comment
There was a problem hiding this comment.
Thank you! This is so much clearer.
| _Note this matches how auth server unit tests jobs in CI._ | ||
|
|
||
| To run a certain suite of tests (e.g. all remote tests): | ||
| For general development based testing, specific can be targeted using the test script or use mocha directly: |
|
|
||
| - Note: stop the auth-server before running tests. Otherwise, they will fail with obscure errors. | ||
| - You can use `LOG_LEVEL`, such as `LOG_LEVEL=debug` to specify the test logging level. | ||
| - For quick enviroment config, consider running tests with a .env file and the dotenv command. For example: `dotenv -- yarn workspace fxa-auth-server:test-integration remote` |
There was a problem hiding this comment.
He does mention below: recovery-phone tests require twilio testing credentials!
I think those might be the only ones?
dschom
force-pushed
the
better-auth-testing-readme
branch
from
789cb02 to
e351f00
Compare
3 participants
Because
This pull request
Issue that this pull request solves
Closes: # (issue number)
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.