Test that Enketo IDs are requested during request to create form #1001
Conversation
This is to prevent intermittent test failures: #588 (comment)
These are additional tests of e1b6e7c.
| const asAlice = await service.login('alice'); | ||
| global.enketo.wait = (f) => { setTimeout(f, 501); }; | ||
| global.enketo.wait = (done) => { setTimeout(done, 600); }; |
There was a problem hiding this comment.
The change from 501 to 600 is meant to address this intermittent test failure: #588 (comment). Note that this test was not introduced in #989, but rather #782. I don't think I did anything in #989 that would have changed the POST/GET …/draft endpoints. However, if this test is failing, it certainly seems possible that new tests in #989 could fail, so I think it's worth addressing now.
Here's one possible explanation for the failure:
- A request to Enketo is sent. After 501 ms, the Enketo mock will respond.
- A moment later, the endpoint time-bounds the request. After 500 ms has elapsed since that bounding, the endpoint will stop waiting for the request.
- If "a moment later" is greater than 1 ms, then the test will fail.
Another possible explanation:
setTimeout()doesn't run callbacks after an exact number of milliseconds, but after at least the specified number of milliseconds. So the Enketo mock could respond after 502 ms instead of after 501 ms.- Similarly, the endpoint could stop waiting after 502 ms instead of after 500 ms.
- In other words, both timers could exceed the number of milliseconds specified and maybe be processed together in a single batch of timers. In that case, the order in which the callbacks are run might be indeterminate.
I'm not sure what the exact explanation is, but the fact that the test is failing intermittently doesn't make me think that there's anything wrong with the functionality here. I think we just need to provide a little more wiggle room for the test.
| .send(testData.forms.simple2) | ||
| .expect(200); | ||
| global.enketo.callCount.should.equal(1); | ||
| without(['token'], global.enketo.createData).should.eql({ |
There was a problem hiding this comment.
I don't think we need to test the token argument at this time. Unlike in the draft case above, token won't be undefined. However, I'm not sure that the token is even needed for this flow to function: see #997. We don't test the token in existing tests.
| @@ -24,6 +24,8 @@ const defaults = { | |||
| // The number of times that the mock has been called during the test, that is, | |||
| // the number of requests that would be sent to Enketo | |||
| callCount: 0, | |||
| // An object with a property for each argument passed to the create() method | |||
| createData: undefined, | |||
There was a problem hiding this comment.
I added createData instead of using receivedUrl in the new tests here, for two reasons:
- This allows us to test the
xmlFormIdargument ofenketo.create()(the second argument), not just the first argument (openRosaUrl/receivedUrl). - Consistency with
editDataelsewhere in this file, which we use to testenketo.edit().
|
@ktuite, I think this can be reviewed async, but let me know if you'd like to review interactively! |
Follow-up PR to #989, adding more tests. Only test/ is changed, not lib/.
#989 included tests of requests to publish an existing form. This PR also tests requests to create a form (either as a draft form or as an immediately published form).
Before submitting this PR, please make sure you have:
make test-fulland confirmed all checks still pass OR confirm CircleCI build passes