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: moved test-debugger-repeat-last.js from parallel to sequential #12470

Closed
wants to merge 1 commit into from
Closed

test: moved test-debugger-repeat-last.js from parallel to sequential #12470

wants to merge 1 commit into from

Conversation

kumarrishav
Copy link
Contributor

@kumarrishav kumarrishav commented Apr 17, 2017

Refs: #12376

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 17, 2017
@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

It seems we have timeouts with this test now.

@@ -10,7 +10,7 @@ const fixture = path.join(

const args = [
'debug',
`--port=${common.PORT}`,
`--port=${0}`,
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 17, 2017

Choose a reason for hiding this comment

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

If I get it right, this could be just '--port=0' now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. updating it now.

@vsemozhetbyt
Copy link
Contributor

@@ -10,7 +10,7 @@ const fixture = path.join(

const args = [
'debug',
`--port=${common.PORT}`,
`--port=0`,
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 17, 2017

Choose a reason for hiding this comment

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

Sorry, linter fails: Strings must use singlequote Could you replace backticks with singlequotes, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my fault :( . now fixed.
Tests are getting passed on my mac system

@vsemozhetbyt
Copy link
Contributor

cc @nodejs/testing Still timeouts on Linux machines.

@vsemozhetbyt
Copy link
Contributor

@@ -10,7 +10,7 @@ const fixture = path.join(

const args = [
'debug',
`--port=${common.PORT}`,
'--port=0',
Copy link
Member

Choose a reason for hiding this comment

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

This won't work. debug cannot accept port 0 on the command line. It requires a port between 1024 and 65535.

Copy link
Member

Choose a reason for hiding this comment

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

This might be one of those cases where the best thing to do is to move the test from parallel to sequential.

Copy link
Member

Choose a reason for hiding this comment

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

doh! you're right. To be honest, tho, --port=0 is something that I believe should work. That's a separate change tho.

Copy link
Member

Choose a reason for hiding this comment

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

--port=0 is something that I believe should work.

See @bnoordhuis's #5025 (and briefly @sam-github's #12093).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should we go with this? move it sequential ?

Copy link
Member

Choose a reason for hiding this comment

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

How should we go with this? move it sequential ?

Yeah, I think so.

@kumarrishav kumarrishav changed the title test: change the common.PORT to 0 in test-debugger-repeat-last.js test: moved test-debugger-repeat-last.js from parallel to sequential Apr 18, 2017
@kumarrishav
Copy link
Contributor Author

moved the test from parallel to sequential. @Trott

@jasnell
Copy link
Member

jasnell commented Apr 19, 2017

@Trott
Copy link
Member

Trott commented Apr 19, 2017

Lone CI failure is unrelated (and should be fixed now if you want to run CI again, but I don't think that's necessary in this case).

lpinca pushed a commit to lpinca/node that referenced this pull request Apr 22, 2017
PR-URL: nodejs#12470
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@lpinca
Copy link
Member

lpinca commented Apr 22, 2017

Landed in 200961b.

@lpinca lpinca closed this Apr 22, 2017
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
PR-URL: #12470
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
PR-URL: #12470
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12470
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gibfahn pushed a commit that referenced this pull request May 16, 2017
PR-URL: #12470
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
PR-URL: #12470
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
PR-URL: nodejs/node#12470
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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.