-
Notifications
You must be signed in to change notification settings - Fork 6.8k
chore(build): fix karma not exiting properly #1741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
01943fe
to
50bd305
Compare
R: @hansl |
@@ -59,6 +59,7 @@ export function config(config) { | |||
colors: true, | |||
logLevel: config.LOG_INFO, | |||
autoWatch: true, | |||
autoWatchBatchDelay: 1000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1000 might be too slow. 500?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, it would debounce the changes, but since this is only the timeout from the first change to when it reloads, it should be fine. This was the "ideal" one that didn't feel slow and caused the least refreshing.
new karma.Server({ | ||
configFile: path.join(PROJECT_ROOT, 'test/karma.conf.js') | ||
}, done).start(); | ||
}).start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not the right fix. By default, Karma uses process.exit
as the callback. We definitely want to exit Gulp gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can switch it to:
() => {
done();
process.exit();
}
I didn't include it initially, because it seemed redundant since process.exit
kills the process anyway (which includes Gulp).
7c7c37b
to
efa4188
Compare
Updated to both exit Gulp and the process, as well as reduced the |
@crisbeto: That's the thing; we don't want to exit the process. We want Gulp to finish the tasks gracefully. Isn't there a better way to kill the watch tasks? |
I'm not sure interrupting a task while it's running is really an issue here, because Karma always runs after all of the build steps. I don't know of a better way to kill it (considering that Karma's default is Also we should keep in mind that this is mainly for local development. |
@@ -51,7 +51,10 @@ gulp.task(':test:deps:inline', sequenceTask(':test:deps', ':inline-resources')); | |||
gulp.task('test', [':test:watch'], (done: () => void) => { | |||
new karma.Server({ | |||
configFile: path.join(PROJECT_ROOT, 'test/karma.conf.js') | |||
}, done).start(); | |||
}, (exitCode: number) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay you can remove the handler. Seems like there's no way to terminate a watch task... which sucks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* Fixes the Karma tasks not exiting properly. This is an issue, because doing a keyboard interrupt doesn't stop the Gulp watchers that are going on in the background. * Increase the time that Karma waits before starting to run tests. This should help mitigate the multiple reloads when running unit tests locally. Related to angular#1681.
efa4188
to
87f748a
Compare
This reverts commit 77701cc.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Related to #1681.