Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Return exit code 1 when incomplete tasks are detected #145

Merged
merged 7 commits into from
Jan 22, 2018
Merged

Return exit code 1 when incomplete tasks are detected #145

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fabfd90
Return exit code 1 when incomplete tasks are detected
barbogast Jan 13, 2018
74b167d
Don't require process
barbogast Jan 17, 2018
e84bfc6
Add test case for sync-task.js
barbogast Jan 17, 2018
0aade92
Disable test for exit code if nodejs < 0.11.0
barbogast Jan 19, 2018
b4bbee2
Change detection of nodejs version to work without adding a new depen…
barbogast Jan 22, 2018
107c1fc
Move test file from test/lib to test
barbogast Jan 22, 2018
f566b8b
Rename test/sync-task.js to test/non-completing-tasks.js
barbogast Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/versioned/^4.0.0/log/sync-task.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ function warn() {
return tasks[key];
}).join(', ');

process.exitCode = 1;

log.warn(
ansi.red('The following tasks did not complete:'),
ansi.cyan(taskNames)
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't bring in this dep.

},
"keywords": [
"build",
Expand Down
2 changes: 1 addition & 1 deletion test/expected/flags-tasks-json.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"label":"gulp-cli/test/fixtures/gulpfiles","nodes":[{"label":"test1","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"noop","type":"function","nodes":[]}]}]},{"label":"test2","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"test1","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"noop","type":"function","nodes":[]}]}]},{"label":"noop","type":"function","nodes":[]}]}]},{"label":"test3","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"described","type":"function","nodes":[]}]}]},{"label":"test4","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"errorFunction","type":"function","nodes":[]},{"label":"anon","type":"function","nodes":[]}]}]},{"label":"test5","nodes":[],"type":"task"},{"label":"test6","nodes":[],"type":"task"},{"label":"default","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"test1","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"noop","type":"function","nodes":[]}]}]},{"label":"test3","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"described","type":"function","nodes":[]}]}]},{"label":"noop","type":"function","nodes":[]}]}]}]}
{"label":"gulp-cli/test/fixtures/gulpfiles","nodes":[{"label":"test1","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"noop","type":"function","nodes":[]}]}]},{"label":"test2","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"test1","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"noop","type":"function","nodes":[]}]}]},{"label":"noop","type":"function","nodes":[]}]}]},{"label":"test3","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"described","type":"function","nodes":[]}]}]},{"label":"test4","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"errorFunction","type":"function","nodes":[]},{"label":"anon","type":"function","nodes":[]}]}]},{"label":"test5","nodes":[],"type":"task"},{"label":"test6","nodes":[],"type":"task"},{"label":"test7","nodes":[],"type":"task"},{"label":"test8","nodes":[],"type":"task"},{"label":"default","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"test1","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"noop","type":"function","nodes":[]}]}]},{"label":"test3","type":"task","nodes":[{"label":"<series>","type":"function","branch":true,"nodes":[{"label":"described","type":"function","nodes":[]}]}]},{"label":"noop","type":"function","nodes":[]}]}]}]}
2 changes: 2 additions & 0 deletions test/expected/flags-tasks-simple.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,6 @@ test3
test4
test5
test6
test7
test8
default
4 changes: 3 additions & 1 deletion test/expected/flags-tasks.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,6 @@ gulp-cli/test/fixtures/gulpfiles
│ ├── errorFunction
│ └── anon
├── test5
└── test6
├── test6
├── test7
└── test8
11 changes: 11 additions & 0 deletions test/fixtures/gulpfiles/gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ function errorFunction() {
function anon(cb) {
cb();
}

function notCompleting1() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need 2 non-completing tasks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Callback is not called
}

function notCompleting2() {
// Callback is not called
}

described.description = 'description';

gulp.task('test1', gulp.series(noop));
Expand All @@ -28,5 +37,7 @@ gulp.task('test3', gulp.series(described));
gulp.task('test4', gulp.series(errorFunction, anon));
gulp.task('test5', delayed);
gulp.task('test6', noop);
gulp.task('test7', notCompleting1);
gulp.task('test8', notCompleting2);

gulp.task('default', gulp.series('test1', 'test3', noop));
43 changes: 43 additions & 0 deletions test/lib/sync-task.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

var expect = require('expect');
var Semver = require('sver-compat').Semver;
var eraseTime = require('gulp-test-tools').eraseTime;
var runner = require('gulp-test-tools').gulpRunner;


function nodeDoesNotNodeSupportExitCode() {
var currentVersion = new Semver(process.versions.node);
var minimalVersion = new Semver('0.11.0');
return currentVersion.lt(minimalVersion);
}

describe('sync-task', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this instead:

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

  cb();
});

it('should return error code 1 if any tasks did not complete', function(done) {
var this_ = this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
    }
    ...

Copy link
Contributor Author

@barbogast barbogast Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@barbogast barbogast Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@sttk sttk Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

runner({ verbose: false })
.chdir('test/fixtures/gulpfiles')
.gulp('test6 test7 test8')
.run(function(err) {
if (nodeDoesNotNodeSupportExitCode()) {
// The exit code is set to 1 by setting process.exitCode = 1 which is only supported from nodejs 0.11 on
this_.skip();
}
expect(err).toNotEqual(null);
expect(err.code).toEqual(1);
done();
});
});

it('should log tasks which did not complete', function(done) {
runner({ verbose: false })
.chdir('test/fixtures/gulpfiles')
.gulp('test6 test7 test8')
.run(function(err, stdout) {
var lines = stdout.split('\n');
expect(lines.length).toEqual(8);
expect(eraseTime(lines[5])).toEqual('The following tasks did not complete: test7, test8');
done();
});
});
});