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

test: remove hardwired references to 'iojs' #1882

Closed
wants to merge 1 commit into from
Closed

test: remove hardwired references to 'iojs' #1882

wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jun 3, 2015

there shouldn't be any references to 'iojs' in the tests (or 'node' either, but they were mostly taken care of already)

bnoordhuis
bnoordhuis reviewed Jun 3, 2015
View reviewed changes
test/sequential/test-setproctitle.js
@@ -25,7 +26,7 @@ exec('ps -p ' + process.pid + ' -o args=', function(error, stdout, stderr) {
assert.equal(stderr, '');

// freebsd always add ' (procname)' to the process title
if (process.platform === 'freebsd') title += ' (iojs)';
if (process.platform === 'freebsd') `${title} (${path.basename(process.execPath)})`;
Copy link
Member

Choose a reason for hiding this comment

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

This turns it into a no-op, doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

why yes, yes it does .. fixed, thanks

@rvagg
Copy link
Member Author

rvagg commented Jun 3, 2015

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/757/

bnoordhuis
bnoordhuis reviewed Jun 3, 2015
View reviewed changes
test/sequential/test-setproctitle.js
@@ -25,7 +26,7 @@ exec('ps -p ' + process.pid + ' -o args=', function(error, stdout, stderr) {
assert.equal(stderr, '');

// freebsd always add ' (procname)' to the process title
if (process.platform === 'freebsd') title += ' (iojs)';
if (process.platform === 'freebsd') title += ` (${path.basename(process.execPath)})`;
Copy link
Member

Choose a reason for hiding this comment

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

Long line.

@bnoordhuis
Copy link
Member

LGTM sans style nit. Didn't make lint complain about that?

@rvagg
Copy link
Member Author

rvagg commented Jun 3, 2015

pffft, make lint? who runs that?

seriously .. I forget to run it, and it's still not in CI

@rvagg
Copy link
Member Author

rvagg commented Jun 3, 2015

fixed btw

@thefourtheye
Copy link
Contributor

I run make test before submitting the PR and pray to god that there are test cases to test my changes.

@bnoordhuis
Copy link
Member

Linting is part of make test and make test-ci so the CI should catch it (eventually - it runs at the very end.)

@rvagg
Copy link
Member Author

rvagg commented Jun 3, 2015

make test-ci isn't enabled still because of problems with -J, we get errors on many of the build slaves when using it, there's an issue around here somewhere that @jbergstroem is working on, please help if you have time!

silverwind
silverwind reviewed Jun 3, 2015
View reviewed changes
test/sequential/test-setproctitle.js
@@ -25,7 +26,8 @@ exec('ps -p ' + process.pid + ' -o args=', function(error, stdout, stderr) {
assert.equal(stderr, '');

// freebsd always add ' (procname)' to the process title
if (process.platform === 'freebsd') title += ' (iojs)';
if (process.platform === 'freebsd')
title += ` (${path.basename(process.execPath)})`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will append .exe on Windows. I say just change it to node or do something like this.

Copy link
Member

Choose a reason for hiding this comment

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

process.platform can't be 'freebsd' on Windows

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right 😅

@silverwind
Copy link
Contributor

LGTM

@silverwind
Copy link
Contributor

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/758/

@Fishrock123 Fishrock123 added the test Issues and PRs related to the tests. label Jun 3, 2015
rvagg added a commit that referenced this pull request Jun 3, 2015
@rvagg @silverwind
test: remove hardwired references to 'iojs'
f78c722 
PR-URL: #1882
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
@silverwind
Copy link
Contributor

Test failures were unrelated, landed in f78c722

@silverwind silverwind closed this Jun 3, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
@rvagg @andrewdeandrade
test: remove hardwired references to 'iojs'
ea2aead 
PR-URL: nodejs/node#1882
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
@rvagg rvagg mentioned this pull request Jun 11, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rvagg @bnoordhuis @thefourtheye @silverwind @targos @Fishrock123