-
Notifications
You must be signed in to change notification settings - Fork 1.4k
add endpoint testing example to FAQ #402
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
Conversation
readme.md
Outdated
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 either be in a before, or should use t.context over a closure:
test.beforeEach(t => {
const app = express();
app.post('/signup', signupHandler);
t.context.app = app;
});
test('signup: Success', async t => {
const res = await request(t.context.app)
.post('/signup')
...
});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.
or
let app;
test.before(() => {
app = express();
app.post('/signup', signupHandler);
});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.
However my new preference is just to do async safe helper functions:
function app() {
const application = express();
application.post('/signup', signupHandler);
return application;
}
test('signup: Success', async t => {
const res = await request(app())
.post(...)
...
});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 to know, thanks! a before would probably suffice, but I wasn't sure if it was a "best practice" since someone could mutate app within a test.
if you don't mind, could you explain why context is needed & a let doesn't suffice? I'm guessing it's because the let won't carry through to all the child processes?
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.
could you explain why context is needed
Because everything executes concurrently in AVA. Assuming you are coming from mocha, you are accustomed to this order of execution:
beforeEachfortest 1test 1beforeEachfortest 2test 2beforeEachfortest 3test 3
With AVA, your order of operations could be:
beforeEachfortest 2beforeEachfortest 1test 1beforeEachfortest 3test 2test 3
Note it is unlikely the order is exactly as above, the point I'm making is that you can't rely on order of execution. The only ordering you are guaranteed is that beforeEach for test X happens before test X. Where it happens in relation to other tests and their respective beforeEaches is not guaranteed.
If you use the --serial flag (ava --serial my-test.js), then your tests are ordered similar to mocha.
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.
ahhhh, that kinda clears it up. I was thinking that beforeEach was guaranteed to run immediately before the test on that thread. So is beforeEach guaranteed to run on the same thread as the test?
PS, I like your 3rd option of creating the app, I'll update the readme to reflect it
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.
Each test file is it's own process (or thread if you want to think of it that way).
t.context is shared between your beforeEach and the appropriate test. So in the above example, even though beforeEach is executed 3 times, it gets a unique t.context each time that is shared with just a single test run. If you are using a closure to pass data between a beforeEach and a test, that is almost certainly wrong, and should use t.context as well.
It's perfectly acceptable to use a closure to pass data created in t.before (in fact t.context does not work in t.before).
|
@jamestalmage thanks for your thoughtful explanation! Let me know if it looks good & I'll squash the commits. |
|
👍 LGTM If you can figure out a concise way of communicating what I've said in this discussion, that would be a welcome PR as well. It's a point of confusion for a lot of people. |
|
rebased & rewritten to cover our discussion, cheers |
|
Nice, latest changes are even better. |
|
I think this should be in a |
readme.md
Outdated
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.
Link to the GitHub repo, not npm page.
|
@mattkrick - Do you have time to update this PR with the recommended changes? |
|
yep, will get a PR out today, sorry for the delay! |
|
ok, should be good to go |
|
@mattkrick - One last question. I am not familiar enough with the supertest API to tell if the code is correct or not. Can you verify you have successfully tested IRL with code that looks similar to the example provided here? |
|
I like how you think! |
|
👍 LGTM |
docs/recipes/endpoint-testing.md
Outdated
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.
Nitpick, but an empty line between these (after the title and the text),
|
Thank you @mattkrick :) |
fixes #310
full example seen here: https://github.com/mattkrick/meatier/blob/master/src/server/controllers/__tests__/auth-tests.js