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.

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

test: remove hardwired references to 'iojs' #1882

Closed
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)

@@ -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 rvagg force-pushed the process_execPath branch from 98304ca to 8defb39 Compare June 3, 2015 11:57
@rvagg
Copy link
Member Author

rvagg commented Jun 3, 2015

@@ -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 rvagg force-pushed the process_execPath branch from 8defb39 to 42c17f5 Compare June 3, 2015 12:01
@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!

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

@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
PR-URL: #1882
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@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
PR-URL: nodejs/node#1882
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@rvagg rvagg mentioned this pull request Jun 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants