Return exit code 1 when incomplete tasks are detected #145

barbogast · 2018-01-13T18:00:44Z

Looks like this could be a fix for gulpjs/gulp#2081

phated · 2018-01-13T22:05:01Z

Please add tests and make sure you aren't breaking old versions. process isn't a module and is just a global.

sttk · 2018-01-14T04:45:26Z

@phated Should we treat this for gulp v3? It is possible by modifying events.js.

phated · 2018-01-14T05:49:17Z

Sure! Anything that is easily backported should be.

sttk · 2018-01-14T06:33:45Z

@barbogast Could you also implement for gulp v3? Regarding tests on v3, since we will address them in the issue #139, you may write only a unit test for logEvents in events.js.

barbogast · 2018-01-17T08:07:22Z

I just added a test and removed the require('process') call.

I'd try to backport the change to gulp v3 but to me it looks like gulp v3 doesn't detect incomplete tasks but just exits the process.

The integration tests fail for nodejs 0.10 as expected (see gulpjs/gulp#2081 (comment)). Is there a way to skip the test for nodejs 0.10?

phated · 2018-01-17T21:46:24Z

if (!('exitCode' in process)) {
  this.skip();
}

Should work???

barbogast · 2018-01-18T20:49:06Z

I'm pretty sure that this wouldn't solve the problem. The code didn't crash. Only the test failed because the process didn't exit with an error code (as is expected for nodejs 0.10). We need a way to disable this test case for nodejs 0.10. We could parse process.version and skip the test for nodejs < 0.11. Kind of hacky, bit it should work...

phated · 2018-01-18T20:54:47Z

That goes in the test to skip it. The property won't exist in old node

barbogast · 2018-01-18T21:44:59Z

Oh, sorry, I didn't understand. I just checked, and exitCode is not present on process (except when it's set explicitly).

We could detect the node version using https://github.com/phated/sver-compat

sttk · 2018-01-19T12:04:20Z

test/lib/sync-task.js

+
+describe('sync-task', function() {
+  it('should return error code 1 if any tasks did not complete', function(done) {
+    var this_ = this;


@barbogast It's not necessary to use the new package. As @phated said, this test case can be skipped by the following code at this point:

it('should return error code 1 if any tasks did not complete', function(done) { if (!('exitCode' in process)) { // On v0.10 only, this condition is true because process doesn't have .exitCode. this.skip(); return; // This is not necessary because skip() throws an exception, but is recommened to avoid confusing. } ...

Are you sure this will work? I just rechecked and

> 'exitCode' in process false

for

node -v v8.9.3

So even on nodejs > 0.11.0 the exitCode property is not set by default.

Or am I misunderstanding completely?

Ah, I think I understood. Do you mean that exitCode was set already by process.exitCode = 1 in sync-task.js? This would work, but only if require('gulp-test-tools').gulpRunner which is used by the test doesn't spawn a new process, right? I guess that it does but I'll try when I get home.

@barbogast I'm sorry, I really misunderstood. You are right. I did only checked process.exitCode is undefined on v0.10 and I completely thought process.exitCode is zero initially on other versions.

@barbogast Since these test cases in test/lib/sync-task.js are not unit test, please move this file to test/ and rename it to another like exit-code.js.

In addition, I think it's better that these two cases can be put into one case and then nodeDoesNotNodeSupportExitCode() is used not for this.skip() but for unchecking exit code.

Please move the file like sttk mentioned and remove that utility (using my other recommendation above).

phated

A few things

phated · 2018-01-22T19:22:59Z

package.json

@@ -64,7 +64,8 @@
    "marked-man": "^0.1.3",
    "mocha": "^3.2.0",
    "nyc": "^10.0.0",
-    "rimraf": "^2.6.1"
+    "rimraf": "^2.6.1",
+    "sver-compat": "^1.5.0"


Don't bring in this dep.

phated · 2018-01-22T19:25:51Z

test/lib/sync-task.js

+  return currentVersion.lt(minimalVersion);
+}
+
+describe('sync-task', function() {


Use this instead:

beforeEach(function(cb) { if (process.version.slice(5) === 'v0.10') { this.skip(); } cb(); });

phated · 2018-01-22T19:26:27Z

test/lib/sync-task.js

+
+describe('sync-task', function() {
+  it('should return error code 1 if any tasks did not complete', function(done) {
+    var this_ = this;


Please move the file like sttk mentioned and remove that utility (using my other recommendation above).

phated · 2018-01-22T19:27:16Z

test/fixtures/gulpfiles/gulpfile.js

@@ -20,6 +20,15 @@ function errorFunction() {
 function anon(cb) {
  cb();
 }
+
+function notCompleting1() {


Do we need 2 non-completing tasks?

I created 2 tasks to also test that the log message is rendered correctly with multiple non-completing tasks. Should I remove the second task?

That's fine. I'm not partial either way.

…dency

barbogast · 2018-01-22T20:20:09Z

Thank you for your review. 👍

The new dependency and the utility function are removed.

Note that in addition to a test for the exit code I added a test for the log message about the non-completing tests. I couldn't find any tests for sync-tasks.js so I went ahead and added a second test case.

The reason I created 2 test cases is that test case for the log message can run under nodejs 0.10 and should be reported as successful while the second test case is then reported as skipped. Should I join them anyway?

The test file is moved to the test-directory. I have not yet renamed the test file since it not only tests the exit code but also the log message for non-completing tests. So I think the name exit-code.js is not fitting. Maybe non-completing-tasks.js?

phated · 2018-01-22T20:37:25Z

non-completing-tasks.js sounds great!

barbogast · 2018-01-22T20:42:30Z

all right, I'll rename it

phated · 2018-01-22T20:50:20Z

This PR looks great @barbogast - I'm just going to wait until the CI complete then merge and publish

barbogast · 2018-01-22T20:55:43Z

Great, thanks a lot for your reviews, @phated @sttk

Should I sqash the commits before merging?

phated · 2018-01-22T20:57:18Z

Should I squash the commits before merging?

Nah, I'll do that in the merge.

sttk · 2018-01-22T21:03:20Z

@barbogast Great! Thank you for your contribution.

phated · 2018-01-22T22:30:59Z

Published as 2.0.1 - Thanks again @barbogast!!

Return exit code 1 when incomplete tasks are detected

fabfd90

barbogast added 2 commits January 17, 2018 08:43

Don't require process

74b167d

Add test case for sync-task.js

e84bfc6

Disable test for exit code if nodejs < 0.11.0

0aade92

sttk reviewed Jan 19, 2018

View reviewed changes

sttk mentioned this pull request Jan 20, 2018

Return exit code 1 when incomplete tasks are detected on gulp v3 #149

Closed

phated requested changes Jan 22, 2018

View reviewed changes

barbogast added 2 commits January 22, 2018 21:07

Change detection of nodejs version to work without adding a new depen…

b4bbee2

…dency

Move test file from test/lib to test

107c1fc

phated approved these changes Jan 22, 2018

View reviewed changes

Rename test/sync-task.js to test/non-completing-tasks.js

f566b8b

phated merged commit f57134a into gulpjs:master Jan 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return exit code 1 when incomplete tasks are detected #145

Return exit code 1 when incomplete tasks are detected #145

barbogast commented Jan 13, 2018 •

edited

phated commented Jan 13, 2018

sttk commented Jan 14, 2018

phated commented Jan 14, 2018

sttk commented Jan 14, 2018

barbogast commented Jan 17, 2018

phated commented Jan 17, 2018

barbogast commented Jan 18, 2018

phated commented Jan 18, 2018

barbogast commented Jan 18, 2018

sttk Jan 19, 2018

barbogast Jan 19, 2018 •

edited

barbogast Jan 19, 2018 •

edited

sttk Jan 19, 2018 •

edited

sttk Jan 22, 2018

phated Jan 22, 2018

phated left a comment

phated Jan 22, 2018

phated Jan 22, 2018

phated Jan 22, 2018

phated Jan 22, 2018

barbogast Jan 22, 2018

phated Jan 22, 2018

barbogast commented Jan 22, 2018

phated commented Jan 22, 2018

barbogast commented Jan 22, 2018

phated commented Jan 22, 2018

barbogast commented Jan 22, 2018

phated commented Jan 22, 2018

sttk commented Jan 22, 2018

phated commented Jan 22, 2018

Return exit code 1 when incomplete tasks are detected #145

Return exit code 1 when incomplete tasks are detected #145

Conversation

barbogast commented Jan 13, 2018 • edited

phated commented Jan 13, 2018

sttk commented Jan 14, 2018

phated commented Jan 14, 2018

sttk commented Jan 14, 2018

barbogast commented Jan 17, 2018

phated commented Jan 17, 2018

barbogast commented Jan 18, 2018

phated commented Jan 18, 2018

barbogast commented Jan 18, 2018

Choose a reason for hiding this comment

barbogast Jan 19, 2018 • edited

Choose a reason for hiding this comment

barbogast Jan 19, 2018 • edited

Choose a reason for hiding this comment

sttk Jan 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbogast commented Jan 22, 2018

phated commented Jan 22, 2018

barbogast commented Jan 22, 2018

phated commented Jan 22, 2018

barbogast commented Jan 22, 2018

phated commented Jan 22, 2018

sttk commented Jan 22, 2018

phated commented Jan 22, 2018

barbogast commented Jan 13, 2018 •

edited

barbogast Jan 19, 2018 •

edited

barbogast Jan 19, 2018 •

edited

sttk Jan 19, 2018 •

edited