Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Make flawed integration tests explode by fixing nock expectations
Up until now, a handful of integration tests has been flawed because
we didn't actually verify all nock.js expectations (read: nock.js scope)
had been fulfilled. That was due to a desire to run several methods as
a one-liner instead of one per line;

```
filesScope.done() && existingRepoLabelsScope.done()
```

That could make sense at first glance, but there's a very important
catch we didn't think about when writing it; it requires `.done()`
to return a truthy value or else what comes after `.done()` will not
be executed.

The `.done()` method on nock.js' scope either throws an exception if
the expectation it represents has not been met, or returns
`undefined` when all is good.

That `undefined` value stops the subsequent methods on the same line to
be executed. In other words, we have always just checked the first
expectation, and none of the subsequent ones on the same line.

The changes introduced in this commit executes these `.done()`s on their
own line which sadly causes all of the related tests to explode at
the moment.

Why most of these expectations haven't been met is probably due to
timing issues, since we don't wait for all the related code to have
finished executing before the tests are run. As of now, the code has
been written in a fashion that allows incoming HTTPS requests to be get
their response ASAP, whilst the code pushing PR statuses to github.com
or trigger Jenins builds are still running. That raises some challenges
for our integration tests since they don't really know when the code is
finished, meaning tests can run.

Upcoming changes will fix that by ensuring incoming requests will get
their response *after* all relevant code has succeeded or failed.
That will introduce quite a big refactor job, but the end result will
be a lot more robust tests that can be trusted.
  • Loading branch information
phillipj committed Apr 10, 2020
commit 249b0622d15d67a17fa5ceaee8e36f9f848d226d
27 changes: 23 additions & 4 deletions test/integration/node-labels-webhook.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ tap.test('Sends POST request to https://api.github.com/repos/nodejs/node/issues/
.reply(200)

t.plan(1)
t.tearDown(() => filesScope.done() && existingRepoLabelsScope.done() && newLabelsScope.done() && clock.uninstall())
t.tearDown(() => {
filesScope.done()
existingRepoLabelsScope.done()
newLabelsScope.done()
clock.uninstall()
})

supertest(app)
.post('/hooks/github')
Expand Down Expand Up @@ -78,7 +83,12 @@ tap.test('Adds v6.x label when PR is targeting the v6.x-staging branch', (t) =>
.reply(200)

t.plan(1)
t.tearDown(() => filesScope.done() && existingRepoLabelsScope.done() && newLabelsScope.done() && clock.uninstall())
t.tearDown(() => {
filesScope.done()
existingRepoLabelsScope.done()
newLabelsScope.done()
clock.uninstall()
})

supertest(app)
.post('/hooks/github')
Expand Down Expand Up @@ -107,7 +117,11 @@ tap.test('Does not create labels which does not already exist', (t) => {
.reply(200, readFixture('repo-labels.json'))

t.plan(1)
t.tearDown(() => filesScope.done() && existingRepoLabelsScope.done() && clock.uninstall())
t.tearDown(() => {
filesScope.done()
existingRepoLabelsScope.done()
clock.uninstall()
})

supertest(app)
.post('/hooks/github')
Expand Down Expand Up @@ -142,7 +156,12 @@ tap.test('Adds V8 Engine label when PR has deps/v8 file changes', (t) => {
.reply(200)

t.plan(1)
t.tearDown(() => filesScope.done() && existingRepoLabelsScope.done() && newLabelsScope.done() && clock.uninstall())
t.tearDown(() => {
filesScope.done()
existingRepoLabelsScope.done()
newLabelsScope.done()
clock.uninstall()
})

supertest(app)
.post('/hooks/github')
Expand Down
7 changes: 6 additions & 1 deletion test/integration/trigger-jenkins-build.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ tap.test('Sends POST request to https://ci.nodejs.org', (t) => {
.reply(200)

t.plan(1)
t.tearDown(() => collaboratorsScope.done() && ciJobScope.done() && commentScope.done() && clock.uninstall())
t.tearDown(() => {
collaboratorsScope.done()
ciJobScope.done()
commentScope.done()
clock.uninstall()
})

supertest(app)
.post('/hooks/github')
Expand Down