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

doc: recompile when testing in CONTRIBUTING.md #2051

Closed
wants to merge 1 commit into from
Closed

doc: recompile when testing in CONTRIBUTING.md #2051

wants to merge 1 commit into from

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

targos commented Jun 24, 2015

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

@brendanashworth brendanashworth 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
Member Author

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

targos commented Jun 25, 2015

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. :)

@phillipj
Copy link
Member Author

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?

@rvagg rvagg force-pushed the master branch 2 times, most recently from 9d21135 to 628a3ab Compare June 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.

@phillipj
Copy link
Member Author

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

@phillipj
doc: recompile when testing in CONTRIBUTING.md
b8789d9 
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
@phillipj
Copy link
Member Author

phillipj commented Jul 8, 2015

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
Member Author

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
@phillipj @thefourtheye
doc: recompile before testing core module changes
c7d8b09 
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 <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@thefourtheye
Copy link
Contributor

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

@phillipj
Copy link
Member Author

Reworded the commit message

👍 and thanks :)

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

targos commented Jul 19, 2015

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

@targos
Copy link
Member

targos commented Jul 19, 2015

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

targos commented Jul 19, 2015

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

@cjihrig cjihrig mentioned this pull request Jul 24, 2015
Merged
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
doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

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