From 1d280e8d928ea27f0c0b904a67c8332760d44cd7 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 8 Oct 2015 10:46:54 +0200 Subject: [PATCH] Introduce timeout option The idea with this commit is to implement a timeout functionality that does not rely on a system utility (such as unix `timeout`). Tests could hang because of a CPU-bound task and, as a result, we need to handle timeouts in a separate process. An ideal implementation would first spawn the timeout, which in turn would spawn the tests, acting as a supervisor and making sure they do not exceed the supplied time. However, setting up such an environment would have been trickier, thus the test process spawns the timeout cop before running any test. This, in turn, invokes a `setTimeout` function and kills the parent process as soon as the timeout gets triggered, logging to console the reason why the parent process was killed (and exiting the parent process with code 130). I haven't used `Task` because, in order to log stuff to the console, we need to fork the ChildProcess from `remote`, as otherwise nothing gets written out. --- src/browser/atom-application.coffee | 10 +++++----- src/browser/atom-window.coffee | 2 +- src/browser/main.coffee | 4 +++- src/initialize-test-window.coffee | 9 +++++++-- src/timeout-cop.js | 12 ++++++++++++ 5 files changed, 28 insertions(+), 9 deletions(-) create mode 100644 src/timeout-cop.js diff --git a/src/browser/atom-application.coffee b/src/browser/atom-application.coffee index c5242c5aa..c7ac7c27a 100644 --- a/src/browser/atom-application.coffee +++ b/src/browser/atom-application.coffee @@ -65,7 +65,7 @@ class AtomApplication exit: (status) -> app.exit(status) constructor: (options) -> - {@resourcePath, @devResourcePath, @version, @devMode, @safeMode, @socketPath} = options + {@resourcePath, @devResourcePath, @version, @devMode, @safeMode, @socketPath, @timeout} = options global.atomApplication = this @@ -87,9 +87,9 @@ class AtomApplication else @loadState() or @openPath(options) - openWithOptions: ({pathsToOpen, executedFrom, urlsToOpen, test, pidToKillWhenClosed, devMode, safeMode, newWindow, specDirectory, logFile, profileStartup}) -> + openWithOptions: ({pathsToOpen, executedFrom, urlsToOpen, test, pidToKillWhenClosed, devMode, safeMode, newWindow, specDirectory, logFile, profileStartup, timeout}) -> if test - @runTests({headless: true, @resourcePath, executedFrom, specDirectory, pathsToOpen, logFile}) + @runTests({headless: true, @resourcePath, executedFrom, specDirectory, pathsToOpen, logFile, timeout}) else if pathsToOpen.length > 0 @openPaths({pathsToOpen, executedFrom, pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup}) else if urlsToOpen.length > 0 @@ -493,7 +493,7 @@ class AtomApplication # :specPath - The directory to load specs from. # :safeMode - A Boolean that, if true, won't run specs from ~/.atom/packages # and ~/.atom/dev/packages, defaults to false. - runTests: ({headless, resourcePath, executedFrom, specDirectory, pathsToOpen, logFile, safeMode}) -> + runTests: ({headless, resourcePath, executedFrom, specDirectory, pathsToOpen, logFile, safeMode, timeout}) -> if resourcePath isnt @resourcePath and not fs.existsSync(resourcePath) resourcePath = @resourcePath @@ -517,7 +517,7 @@ class AtomApplication isSpec = true devMode = true safeMode ?= false - new AtomWindow({windowInitializationScript, resourcePath, headless, isSpec, devMode, testRunnerPath, legacyTestRunnerPath, testPaths, logFile, safeMode}) + new AtomWindow({windowInitializationScript, resourcePath, headless, isSpec, devMode, testRunnerPath, legacyTestRunnerPath, testPaths, logFile, safeMode, timeout}) resolveTestRunnerPath: (testPath) -> FindParentDir ?= require 'find-parent-dir' diff --git a/src/browser/atom-window.coffee b/src/browser/atom-window.coffee index 39350392d..2415e916b 100644 --- a/src/browser/atom-window.coffee +++ b/src/browser/atom-window.coffee @@ -19,7 +19,7 @@ class AtomWindow isSpec: null constructor: (settings={}) -> - {@resourcePath, pathToOpen, locationsToOpen, @isSpec, @headless, @safeMode, @devMode} = settings + {@resourcePath, pathToOpen, locationsToOpen, @isSpec, @headless, @safeMode, @devMode, @timeout} = settings locationsToOpen ?= [{pathToOpen}] if pathToOpen locationsToOpen ?= [] diff --git a/src/browser/main.coffee b/src/browser/main.coffee index f0709037d..b86828392 100644 --- a/src/browser/main.coffee +++ b/src/browser/main.coffee @@ -102,6 +102,7 @@ parseCommandLine = -> options.alias('s', 'spec-directory').string('s').describe('s', 'Set the directory from which to run package specs (default: Atom\'s spec directory).') options.boolean('safe').describe('safe', 'Do not load packages from ~/.atom/packages or ~/.atom/dev/packages.') options.alias('t', 'test').boolean('t').describe('t', 'Run the specified specs and exit with error code on failures.') + options.string('timeout').describe('timeout', 'When in test mode, waits until the specified time (in minutes) and kills the process (exit code: 130).') options.alias('v', 'version').boolean('v').describe('v', 'Print the version.') options.alias('w', 'wait').boolean('w').describe('w', 'Wait for window to be closed before returning.') options.string('socket-path') @@ -121,6 +122,7 @@ parseCommandLine = -> safeMode = args['safe'] pathsToOpen = args._ test = args['test'] + timeout = args['timeout'] specDirectory = args['spec-directory'] newWindow = args['new-window'] pidToKillWhenClosed = args['pid'] if args['wait'] @@ -158,6 +160,6 @@ parseCommandLine = -> {resourcePath, devResourcePath, pathsToOpen, urlsToOpen, executedFrom, test, version, pidToKillWhenClosed, devMode, safeMode, newWindow, specDirectory, - logFile, socketPath, profileStartup} + logFile, socketPath, profileStartup, timeout} start() diff --git a/src/initialize-test-window.coffee b/src/initialize-test-window.coffee index 337c48744..d8354a416 100644 --- a/src/initialize-test-window.coffee +++ b/src/initialize-test-window.coffee @@ -5,6 +5,7 @@ try path = require 'path' ipc = require 'ipc' remote = require 'remote' + app = remote.require('app') {getWindowLoadSettings} = require './window-load-settings-helpers' AtomEnvironment = require '../src/atom-environment' @@ -32,6 +33,12 @@ try document.title = "Spec Suite" + timeoutCop = null + if timeout = getWindowLoadSettings().timeout + ChildProcess = remote.require("child_process") + timeoutCop = ChildProcess.fork(require.resolve('./timeout-cop'), [remote.process.pid, timeout]) + app.on "will-exit", -> timeoutCop.kill() + legacyTestRunner = require(getWindowLoadSettings().legacyTestRunnerPath) testRunner = require(getWindowLoadSettings().testRunnerPath) testRunner({ @@ -41,11 +48,9 @@ try buildAtomEnvironment: -> new AtomEnvironment legacyTestRunner: legacyTestRunner }) - catch error if getWindowLoadSettings().headless console.error(error.stack ? error) - app = remote.require('app') app.emit('will-exit') remote.process.exit(status) else diff --git a/src/timeout-cop.js b/src/timeout-cop.js new file mode 100644 index 000000000..c684540f2 --- /dev/null +++ b/src/timeout-cop.js @@ -0,0 +1,12 @@ +var parentProcessId = process.argv[2]; +var timeoutInMinutes = process.argv[3]; +var timeoutInMilliseconds = timeoutInMinutes * 1000 * 60 + +function exitTestRunner() { + process.kill(parentProcessId, "SIGINT"); + var errorMessage = "The test suite has timed out because it has been running"; + errorMessage += " for more than " + timeoutInMinutes + " minutes."; + console.log(errorMessage); +} + +setTimeout(exitTestRunner, timeoutInMilliseconds);