chore(testrunner): introduce Location class (#1585)

Drive-by: fix an edge when testing continued after termination.
microsoft · Mar 30, 2020 · b6166c9 · b6166c9
1 parent c49b856
commit b6166c9
Show file tree

Hide file tree

Showing 8 changed files with 120 additions and 66 deletions.
diff --git a/test/playwright.spec.js b/test/playwright.spec.js
@@ -144,7 +144,7 @@ module.exports.describe = ({testRunner, product, playwrightPath}) => {
     afterEach(async (state, test) => {
       if (state.browser.contexts().length !== 0) {
         if (test.result === 'ok')
-          console.warn(`\nWARNING: test "${test.fullName}" (${test.location.fileName}:${test.location.lineNumber}) did not close all created contexts!\n`);
+          console.warn(`\nWARNING: test "${test.fullName()}" (${test.location()}) did not close all created contexts!\n`);
         await Promise.all(state.browser.contexts().map(context => context.close()));
       }
       await state.tearDown();

diff --git a/test/utils.js b/test/utils.js
@@ -254,8 +254,8 @@ const utils = module.exports = {
       const url = `https://github.com/Microsoft/playwright/blob/${sha}/${testpath}#L${test.location.lineNumber}`;
       dashboard.reportTestResult({
         testId: test.testId,
-        name: test.location.fileName + ':' + test.location.lineNumber,
-        description: test.fullName,
+        name: test.location().toString(),
+        description: test.fullName(),
         url,
         result: test.result,
       });
@@ -275,7 +275,7 @@ const utils = module.exports = {
         const testId = testIdComponents.join('>');
         const clashingTest = testIds.get(testId);
         if (clashingTest)
-          throw new Error(`Two tests with clashing IDs: ${test.location.fileName}:${test.location.lineNumber} and ${clashingTest.location.fileName}:${clashingTest.location.lineNumber}`);
+          throw new Error(`Two tests with clashing IDs: ${test.location()} and ${clashingTest.location()}`);
         testIds.set(testId, test);
         test.testId = testId;
       }

diff --git a/utils/testrunner/Location.js b/utils/testrunner/Location.js
@@ -0,0 +1,85 @@
+/**
+ * Copyright 2017 Google Inc. All rights reserved.
+ * Modifications copyright (c) Microsoft Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+ const path = require('path');
+
+class Location {
+  constructor() {
+    this._fileName = '';
+    this._filePath = '';
+    this._lineNumber = 0;
+    this._columnNumber = 0;
+  }
+
+  fileName() {
+    return this._fileName;
+  }
+
+  filePath() {
+    return this._filePath;
+  }
+
+  lineNumber() {
+    return this._lineNumber;
+  }
+
+  columnNumber() {
+    return this._columnNumber;
+  }
+
+  toString() {
+    return this._fileName + ':' + this._lineNumber;
+  }
+
+  toDetailedString() {
+    return this._fileName + ':' + this._lineNumber + ':' + this._columnNumber;
+  }
+
+  static getCallerLocation(filename) {
+    const error = new Error();
+    const stackFrames = error.stack.split('\n').slice(1);
+    const location = new Location();
+    // Find first stackframe that doesn't point to this file.
+    for (let frame of stackFrames) {
+      frame = frame.trim();
+      if (!frame.startsWith('at '))
+        return null;
+      if (frame.endsWith(')')) {
+        const from = frame.indexOf('(');
+        frame = frame.substring(from + 1, frame.length - 1);
+      } else {
+        frame = frame.substring('at '.length);
+      }
+
+      const match = frame.match(/^(.*):(\d+):(\d+)$/);
+      if (!match)
+        return null;
+      const filePath = match[1];
+      if (filePath === __filename || filePath === filename)
+        continue;
+
+      location._filePath = filePath;
+      location._fileName = filePath.split(path.sep).pop();
+      location._lineNumber = parseInt(match[2], 10);
+      location._columnNumber = parseInt(match[3], 10);
+      return location;
+    }
+    return location;
+  }
+}
+
+module.exports = Location;
diff --git a/utils/testrunner/Matchers.js b/utils/testrunner/Matchers.js
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-const {getCallerLocation} = require('./utils.js');
+const Location = require('./Location.js');
 const colors = require('colors/safe');
 const Diff = require('text-diff');
 
@@ -40,7 +40,7 @@ class MatchError extends Error {
     super(message);
     this.name = this.constructor.name;
     this.formatter = formatter;
-    this.location = getCallerLocation(__filename);
+    this.location = Location.getCallerLocation(__filename);
     Error.captureStackTrace(this, this.constructor);
   }
 }

diff --git a/utils/testrunner/Reporter.js b/utils/testrunner/Reporter.js
@@ -198,21 +198,22 @@ class Reporter {
     } else if (testRun.result() === 'failed') {
       console.log(`${prefix} ${colors.red('[FAIL]')} ${test.fullName()} (${formatLocation(test.location())})`);
       if (testRun.error() instanceof MatchError) {
-        let lines = this._filePathToLines.get(testRun.error().location.filePath);
+        const location = testRun.error().location;
+        let lines = this._filePathToLines.get(location.filePath());
         if (!lines) {
           try {
-            lines = fs.readFileSync(testRun.error().location.filePath, 'utf8').split('\n');
+            lines = fs.readFileSync(location.filePath(), 'utf8').split('\n');
           } catch (e) {
             lines = [];
           }
-          this._filePathToLines.set(testRun.error().location.filePath, lines);
+          this._filePathToLines.set(location.filePath(), lines);
         }
-        const lineNumber = testRun.error().location.lineNumber;
+        const lineNumber = location.lineNumber();
         if (lineNumber < lines.length) {
           const lineNumberLength = (lineNumber + 1 + '').length;
-          const FROM = Math.max(test.location().lineNumber - 1, lineNumber - 5);
+          const FROM = Math.max(test.location().lineNumber() - 1, lineNumber - 5);
           const snippet = lines.slice(FROM, lineNumber).map((line, index) => `    ${(FROM + index + 1 + '').padStart(lineNumberLength, ' ')} | ${line}`).join('\n');
-          const pointer = `    ` + ' '.repeat(lineNumberLength) + '   ' + '~'.repeat(testRun.error().location.columnNumber - 1) + '^';
+          const pointer = `    ` + ' '.repeat(lineNumberLength) + '   ' + '~'.repeat(location.columnNumber() - 1) + '^';
           console.log('\n' + snippet + '\n' + colors.grey(pointer) + '\n');
         }
         console.log(padLines(testRun.error().formatter(), 4));
@@ -228,10 +229,10 @@ class Reporter {
           console.log('  Stack:');
           let stack = testRun.error().stack;
           // Highlight first test location, if any.
-          const match = stack.match(new RegExp(test.location().filePath + ':(\\d+):(\\d+)'));
+          const match = stack.match(new RegExp(test.location().filePath() + ':(\\d+):(\\d+)'));
           if (match) {
             const [, line, column] = match;
-            const fileName = `${test.location().fileName}:${line}:${column}`;
+            const fileName = `${test.location().fileName()}:${line}:${column}`;
             stack = stack.substring(0, match.index) + stack.substring(match.index).replace(fileName, colors.yellow(fileName));
           }
           console.log(padLines(stack, 4));
@@ -249,7 +250,7 @@ class Reporter {
 function formatLocation(location) {
   if (!location)
     return '';
-  return colors.yellow(`${location.fileName}:${location.lineNumber}:${location.columnNumber}`);
+  return colors.yellow(`${location.toDetailedString()}`);
 }
 
 function padLines(text, spaces = 0) {

diff --git a/utils/testrunner/TestRunner.js b/utils/testrunner/TestRunner.js
@@ -1,5 +1,6 @@
 /**
  * Copyright 2017 Google Inc. All rights reserved.
+ * Modifications copyright (c) Microsoft Corporation.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,7 +18,7 @@
 const EventEmitter = require('events');
 const {SourceMapSupport} = require('./SourceMapSupport');
 const debug = require('debug');
-const {getCallerLocation} = require('./utils');
+const Location = require('./Location');
 
 const INFINITE_TIMEOUT = 100000000;
 const TimeoutError = new Error('Timeout');
@@ -53,7 +54,7 @@ const TestResult = {
 };
 
 function createHook(callback, name) {
-  const location = getCallerLocation(__filename);
+  const location = Location.getCallerLocation(__filename);
   return { name, body: callback, location };
 }
 
@@ -456,14 +457,13 @@ class TestWorker {
     this._runningHookTerminate = null;
 
     if (error) {
-      const locationString = `${hook.location.fileName}:${hook.location.lineNumber}:${hook.location.columnNumber}`;
       if (testRun && testRun._result !== TestResult.Terminated) {
         // Prefer terminated result over any hook failures.
         testRun._result = error === TerminatedError ? TestResult.Terminated : TestResult.Crashed;
       }
       let message;
       if (error === TimeoutError) {
-        message = `${locationString} - Timeout Exceeded ${timeout}ms while running "${hook.name}" in "${fullName}"`;
+        message = `${hook.location.toDetailedString()} - Timeout Exceeded ${timeout}ms while running "${hook.name}" in "${fullName}"`;
         error = null;
       } else if (error === TerminatedError) {
         // Do not report termination details - it's just noise.
@@ -472,7 +472,7 @@ class TestWorker {
       } else {
         if (error.stack)
           await this._testPass._runner._sourceMapSupport.rewriteStackTraceWithSourceMaps(error);
-        message = `${locationString} - FAILED while running "${hook.name}" in suite "${fullName}": `;
+        message = `${hook.location.toDetailedString()} - FAILED while running "${hook.name}" in suite "${fullName}": `;
       }
       await this._didFailHook(hook, fullName, message, error);
       if (testRun)
@@ -497,26 +497,26 @@ class TestWorker {
   }
 
   async _willStartTestBody(testRun) {
-    debug('testrunner:test')(`[${this._workerId}] starting "${testRun.test().fullName()}" (${testRun.test().location().fileName + ':' + testRun.test().location().lineNumber})`);
+    debug('testrunner:test')(`[${this._workerId}] starting "${testRun.test().fullName()}" (${testRun.test().location()})`);
   }
 
   async _didFinishTestBody(testRun) {
-    debug('testrunner:test')(`[${this._workerId}] ${testRun._result.toUpperCase()} "${testRun.test().fullName()}" (${testRun.test().location().fileName + ':' + testRun.test().location().lineNumber})`);
+    debug('testrunner:test')(`[${this._workerId}] ${testRun._result.toUpperCase()} "${testRun.test().fullName()}" (${testRun.test().location()})`);
   }
 
   async _willStartHook(hook, fullName) {
-    debug('testrunner:hook')(`[${this._workerId}] "${hook.name}" started for "${fullName}" (${hook.location.fileName + ':' + hook.location.lineNumber})`);
+    debug('testrunner:hook')(`[${this._workerId}] "${hook.name}" started for "${fullName}" (${hook.location})`);
   }
 
   async _didFailHook(hook, fullName, message, error) {
-    debug('testrunner:hook')(`[${this._workerId}] "${hook.name}" FAILED for "${fullName}" (${hook.location.fileName + ':' + hook.location.lineNumber})`);
+    debug('testrunner:hook')(`[${this._workerId}] "${hook.name}" FAILED for "${fullName}" (${hook.location})`);
     if (message)
       this._testPass._result.addError(message, error, this);
     this._testPass._result.setResult(TestResult.Crashed, message);
   }
 
   async _didCompleteHook(hook, fullName) {
-    debug('testrunner:hook')(`[${this._workerId}] "${hook.name}" OK for "${fullName}" (${hook.location.fileName + ':' + hook.location.lineNumber})`);
+    debug('testrunner:hook')(`[${this._workerId}] "${hook.name}" OK for "${fullName}" (${hook.location})`);
   }
 
   async shutdown() {
@@ -581,7 +581,7 @@ class TestPass {
   async _runWorker(testRunIndex, testRuns, parallelIndex) {
     let worker = new TestWorker(this, this._nextWorkerId++, parallelIndex);
     this._workers[parallelIndex] = worker;
-    while (!worker._terminating) {
+    while (!this._terminating) {
       let skipped = 0;
       while (skipped < testRuns.length && testRuns[testRunIndex]._result !== null) {
         testRunIndex = (testRunIndex + 1) % testRuns.length;
@@ -613,6 +613,7 @@ class TestPass {
 
   async _terminate(result, message, force, error) {
     debug('testrunner')(`TERMINATED result = ${result}, message = ${message}`);
+    this._terminating = true;
     for (const worker of this._workers)
       worker.terminate(force /* terminateHooks */);
     this._result.setResult(result, message);
@@ -638,8 +639,7 @@ class TestRunner extends EventEmitter {
     } = options;
     this._crashIfTestsAreFocusedOnCI = crashIfTestsAreFocusedOnCI;
     this._sourceMapSupport = new SourceMapSupport();
-    const dummyLocation = { fileName: '', filePath: '', lineNumber: 0, columnNumber: 0 };
-    this._rootSuite = new Suite(null, '', dummyLocation);
+    this._rootSuite = new Suite(null, '', new Location());
     this._currentSuite = this._rootSuite;
     this._tests = [];
     this._suites = [];
@@ -670,7 +670,7 @@ class TestRunner extends EventEmitter {
 
   _suiteBuilder(callbacks) {
     return new Proxy((name, callback, ...suiteArgs) => {
-      const location = getCallerLocation(__filename);
+      const location = Location.getCallerLocation(__filename);
       const suite = new Suite(this._currentSuite, name, location);
       for (const { callback, args } of callbacks)
         callback(suite, ...args);
@@ -692,7 +692,7 @@ class TestRunner extends EventEmitter {
 
   _testBuilder(callbacks) {
     return new Proxy((name, callback) => {
-      const location = getCallerLocation(__filename);
+      const location = Location.getCallerLocation(__filename);
       const test = new Test(this._currentSuite, name, callback, location);
       test.setTimeout(this._timeout);
       for (const { callback, args } of callbacks)

diff --git a/utils/testrunner/test/testrunner.spec.js b/utils/testrunner/test/testrunner.spec.js
@@ -21,10 +21,10 @@ module.exports.addTests = function({testRunner, expect}) {
       expect(test.fullName()).toBe('uno');
       expect(test.focused()).toBe(false);
       expect(test.skipped()).toBe(false);
-      expect(test.location().filePath).toEqual(__filename);
-      expect(test.location().fileName).toEqual('testrunner.spec.js');
-      expect(test.location().lineNumber).toBeTruthy();
-      expect(test.location().columnNumber).toBeTruthy();
+      expect(test.location().filePath()).toEqual(__filename);
+      expect(test.location().fileName()).toEqual('testrunner.spec.js');
+      expect(test.location().lineNumber()).toBeTruthy();
+      expect(test.location().columnNumber()).toBeTruthy();
     });
     it('should run a test', async() => {
       const t = newTestRunner();

diff --git a/utils/testrunner/utils.js b/utils/testrunner/utils.js