Make flawed integration tests explode by fixing nock expectations

phillipj · phillipj · commit 249b0622d15d · 2020-04-10T13:35:07.000+02:00
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() &amp;&amp; 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.
diff --git a/test/integration/node-labels-webhook.test.js b/test/integration/node-labels-webhook.test.js
@@ -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')
@@ -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')
@@ -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')
@@ -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')
diff --git a/test/integration/trigger-jenkins-build.test.js b/test/integration/trigger-jenkins-build.test.js
@@ -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')
