From 568c6cbb5493e0a614d202d9a170ba0115a150bc Mon Sep 17 00:00:00 2001
From: Joel Einbinder <joel.einbinder@gmail.com>
Date: Wed, 19 Feb 2020 14:26:02 -0800
Subject: [PATCH] test(navigation): fix flaky networkidle tests (#1058)

The network idle tests were waiting for requests to appear on the server, but not for playwright to be notified of the request via protocol. They also assumed performance.now would match up with setTimeout times.
---
 test/navigation.spec.js | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/test/navigation.spec.js b/test/navigation.spec.js
index 6261829630..3c94b42b94 100644
--- a/test/navigation.spec.js
+++ b/test/navigation.spec.js
@@ -16,7 +16,6 @@
  */
 
 const utils = require('./utils');
-const { performance } = require('perf_hooks');
 
 /**
  * @type {PageTestSuite}
@@ -396,29 +395,39 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
         expect(response.status()).toBe(200);
       });
 
+      /**
+       * @param {import('../src/frames').Frame} frame 
+       * @param {TestServer} server 
+       * @param {'networkidle0'|'networkidle2'} signal 
+       * @param {() => Promise<void>} action 
+       * @param {boolean} isSetContent 
+       */
       async function networkIdleTest(frame, server, signal, action, isSetContent) {
-        let lastResponseFinished;
         const finishResponse = response => {
-          lastResponseFinished = performance.now();
           response.statusCode = 404;
           response.end(`File not found`);
         };
-
+        const waitForRequest = suffix => {
+          return Promise.all([
+            server.waitForRequest(suffix),
+            frame._page.waitForRequest(server.PREFIX + suffix),
+          ])
+        }
         let responses = {};
         // Hold on to a bunch of requests without answering.
         server.setRoute('/fetch-request-a.js', (req, res) => responses.a = res);
         server.setRoute('/fetch-request-b.js', (req, res) => responses.b = res);
         server.setRoute('/fetch-request-c.js', (req, res) => responses.c = res);
         const initialFetchResourcesRequested = Promise.all([
-          server.waitForRequest('/fetch-request-a.js'),
-          server.waitForRequest('/fetch-request-b.js'),
-          server.waitForRequest('/fetch-request-c.js'),
+          waitForRequest('/fetch-request-a.js'),
+          waitForRequest('/fetch-request-b.js'),
+          waitForRequest('/fetch-request-c.js')
         ]);
 
         let secondFetchResourceRequested;
         if (signal === 'networkidle0') {
           server.setRoute('/fetch-request-d.js', (req, res) => responses.d = res);
-          secondFetchResourceRequested = server.waitForRequest('/fetch-request-d.js');
+          secondFetchResourceRequested = waitForRequest('/fetch-request-d.js');
         }
 
         const waitForLoadPromise = isSetContent ? Promise.resolve() : frame.waitForNavigation({ waitUntil: 'load' });
@@ -442,10 +451,11 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
         expect(responses.a).toBeTruthy();
         expect(responses.b).toBeTruthy();
         expect(responses.c).toBeTruthy();
-        // Finishing first response should leave 2 requests alive and trigger networkidle2.
-        finishResponse(responses.a);
-
+        let timer;
+        let timerTriggered = false;
         if (signal === 'networkidle0') {
+          // Finishing first response should leave 2 requests alive and trigger networkidle2.
+          finishResponse(responses.a);
           // Finishing two more responses should trigger the second round.
           finishResponse(responses.b);
           finishResponse(responses.c);
@@ -454,11 +464,16 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
           await secondFetchResourceRequested;
           expect(actionFinished).toBe(false);
           // Finishing the last response should trigger networkidle0.
+          timer = setTimeout(() => timerTriggered = true, 500);
           finishResponse(responses.d);
+        } else {
+          timer = setTimeout(() => timerTriggered = true, 500);
+          // Finishing first response should leave 2 requests alive and trigger networkidle2.
+          finishResponse(responses.a);
         }
-
         const response = await actionPromise;
-        expect(performance.now() - lastResponseFinished).not.toBeLessThan(450);
+        clearTimeout(timer);
+        expect(timerTriggered).toBe(true);
         if (!isSetContent)
           expect(response.ok()).toBe(true);