Skip to content

Conversation

@phillipj
Copy link
Member

Changes to core modules does not take effect before project gets recompiled. This was not obvious to me when trying to run some tests at first, which made me bother you with an issue a few weeks ago.

Added a line tipping new contributors about this behavior when describing how to to run tests in contribution guide.

Removed jslint from first test command example as jslint are included when running make test.

Fixed wrong path of example stream2-transform test.

@targos
Copy link
Member

👍
It may not be obvious to the newcomer (at least it was not to me) that the core library is bundled in the executable.

Maybe you could also fix the command right before from
$ iojs ./test/parallel/test-streams2-transform.js
to
$ ./iojs ./test/parallel/test-streams2-transform.js
The former would use the installed version instead of the newly built one.

@brendanashworthbrendanashworth added the doc Issues and PRs related to the documentations. label Jun 24, 2015
@Fishrock123
Copy link
Contributor

+1 to @targos's suggestion.

Also, it may be better to just have ./configure && make -j (-j runs in maximum parallelism i.e. faster), what do you think? :)

@phillipj
Copy link
MemberAuthor

Nice one @targos, I'll fix it shortly.

👍 for make -j. Would running ./configure at every recompile have any benefits compared to prepending it to make jslint test@Fishrock123?

@targos
Copy link
Member

I missed something in my suggestion, there is a typo in the file name.
It should be test-stream2-transform.js.

@Fishrock123
Copy link
Contributor

@phillipj not too much, no. Also make test already includes jslint. :)

@phillipjphillipjforce-pushed the recompile-between-tests branch from cc31ed1 to 25be43eCompareJune 25, 2015 07:43
@phillipj
Copy link
MemberAuthor

Somewhat concerned about advocating maximum parallelism with make as it made my machine come to a grinding halt. make -j8 works perfectly for me, but it depends on individual developer's hardware ofcourse. Any thoughts @Fishrock123?

@rvaggrvaggforce-pushed the master branch 2 times, most recently from 9d21135 to 628a3abCompareJune 25, 2015 09:18
@Fishrock123
Copy link
Contributor

Hmmm, I've never actually just run make -j, but I think it should run at the number of threads you have?

I'm in favor of just suggesting make -j 8 I think.

@phillipjphillipjforce-pushed the recompile-between-tests branch from 25be43e to 4ee6be4CompareJune 28, 2015 21:30
@phillipj
Copy link
MemberAuthor

Alright, that should prevent devs getting load of 300 and a non-responding OS as the plain -j did for me.

Changes to core modules does not take effect before project gets recompiled. Tip new contributors about this when describing how to to run tests in contribution guide. Removed `jslint` from first test command example as jslint are included when running `make test`. Fixed wrong path of example stream2-transform test. PR-URL: nodejs#2051
@phillipjphillipjforce-pushed the recompile-between-tests branch from 4ee6be4 to b8789d9CompareJuly 8, 2015 18:30
@phillipj
Copy link
MemberAuthor

I just added PR-URL to the commit.

@bnoordhuis
Copy link
Member

LGTM. Is there a reason this hasn't been merged yet?

@Fishrock123
Copy link
Contributor

Is there a reason this hasn't been merged yet?

Not really

@thefourtheye
Copy link
Contributor

LGTM too.

@phillipj
Copy link
MemberAuthor

Just say the word if there's anything I should do to get this landed.

thefourtheye pushed a commit that referenced this pull request Jul 19, 2015
Changes to core modules do not take effect unless recompiled. Tip new contributors about this when describing how to run tests in contribution guide. Removed `jslint` from first test command example, as jslint is included when running `make test`. Fixed wrong path of example stream2-transform test. PR-URL: #2051 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@thefourtheye
Copy link
Contributor

@phillipj Reworded the commit message and landed in c7d8b09. Thanks :-)

@phillipj
Copy link
MemberAuthor

Reworded the commit message

👍 and thanks :)

@phillipjphillipj deleted the recompile-between-tests branch July 19, 2015 11:42
@targos
Copy link
Member

@thefourtheye is there something wrong with your system time? c7d8b09 was committed in the future (today, 5pm UTC)

@targos
Copy link
Member

it could also be the reason why your commit wasn't automatically taken for the 2.4.0 changelog

@thefourtheye
Copy link
Contributor

@targos Oh, I am not sure what is wrong. I resynced my machine's time now. Let's see if this happens again. But that commit was landed after 2.4.0 build started, right?

@targos
Copy link
Member

@thefourtheye I don't think so, it landed before mine.

@cjihrigcjihrig mentioned this pull request Jul 24, 2015
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@phillipj@targos@Fishrock123@bnoordhuis@thefourtheye@brendanashworth