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: refactor test-debugger-remote #10455

Closed
wants to merge 1 commit into from
Closed

test: refactor test-debugger-remote #10455

wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Dec 26, 2016 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

  1. The test doesn't attach an event listener for exit events and
    removes them before killing. The intention is to fail the tests if
    the processes exit normally. This patch attaches the exit event
    handlers.

  2. Replace vars with lets and consts.

  3. Replace == based assertion with strictEqual assertion.

  4. Use common.PORT instead of 5959.

  5. The test used to expect only one string "connecting to
    localhost:5959 ... ok", but the debugger actually emits another
    string, "break in test/fixtures/empty.js:2". This patch asserts if
    both of them are received in the same order.

Refer: #10361

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 26, 2016
targos
targos approved these changes Dec 26, 2016
View reviewed changes
test/debugger/test-debugger-remote.js
assert.ok(expected == line, 'Got unexpected line: ' + line);
});
const expected = '\bconnecting to localhost:5959 ... ok';
assert.strictEqual(expected, line, 'Got unexpected line: ' + line);
Copy link
Member

Choose a reason for hiding this comment

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

nit: expected should be the second argument. Also I think the message can be dropped 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.

@targos Done!

@thefourtheye thefourtheye mentioned this pull request Dec 26, 2016
Closed
2 tasks
@thefourtheye
test: refactor test-debugger-remote
a905e57 
1. The test doesn't attach an event listener for `exit` events and
   removes them before killing. The intention is to fail the tests if
   the processes exit normally. This patch attaches the `exit` event
   handlers.

2. Replace `var`s with `let`s and `const`s.

3. Replace `==` based assertion with `strictEqual` assertion.

4. Use `common.PORT` instead of `5959`.

5. The test used to expect only one string "connecting to
   localhost:5959 ... ok", but the debugger actually emits another
   string, "break in test/fixtures/empty.js:2". This patch asserts if
   both of them are received in the same order.

Refer: nodejs#10361
@thefourtheye
Copy link
Contributor Author

I improved the assertions so that problems in CI seen in #10456 will be resolved.

@thefourtheye
Copy link
Contributor Author

Landed in 152bd82

@thefourtheye thefourtheye deleted the refactor-test-debugger-remote branch January 2, 2017 12:44
thefourtheye added a commit that referenced this pull request Jan 2, 2017
@thefourtheye
test: refactor test-debugger-remote
152bd82 
1. The test doesn't attach an event listener for `exit` events and
   removes them before killing. The intention is to fail the tests if
   the processes exit normally. This patch attaches the `exit` event
   handlers.

2. Replace `var`s with `let`s and `const`s.

3. Replace `==` based assertion with `strictEqual` assertion.

4. Use `common.PORT` instead of `5959`.

5. The test used to expect only one string "connecting to
   localhost:5959 ... ok", but the debugger actually emits another
   string, "break in test/fixtures/empty.js:2". This patch asserts if
   both of them are received in the same order.

Refer: #10361
PR-URL: #10455

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@thefourtheye @evanlucas
test: refactor test-debugger-remote
f112a19 
1. The test doesn't attach an event listener for `exit` events and
   removes them before killing. The intention is to fail the tests if
   the processes exit normally. This patch attaches the `exit` event
   handlers.

2. Replace `var`s with `let`s and `const`s.

3. Replace `==` based assertion with `strictEqual` assertion.

4. Use `common.PORT` instead of `5959`.

5. The test used to expect only one string "connecting to
   localhost:5959 ... ok", but the debugger actually emits another
   string, "break in test/fixtures/empty.js:2". This patch asserts if
   both of them are received in the same order.

Refer: #10361
PR-URL: #10455

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@thefourtheye @evanlucas
test: refactor test-debugger-remote
05b0092 
1. The test doesn't attach an event listener for `exit` events and
   removes them before killing. The intention is to fail the tests if
   the processes exit normally. This patch attaches the `exit` event
   handlers.

2. Replace `var`s with `let`s and `const`s.

3. Replace `==` based assertion with `strictEqual` assertion.

4. Use `common.PORT` instead of `5959`.

5. The test used to expect only one string "connecting to
   localhost:5959 ... ok", but the debugger actually emits another
   string, "break in test/fixtures/empty.js:2". This patch asserts if
   both of them are received in the same order.

Refer: #10361
PR-URL: #10455

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins
Copy link
Contributor

This test lands cleanly on v4 and v6 but fails on both. Please feel free to dig in / backport

thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Feb 5, 2017
@thefourtheye
test: refactor test-debugger-remote
fd04f9a 
1. The test doesn't attach an event listener for `exit` events and
   removes them before killing. The intention is to fail the tests if
   the processes exit normally. This patch attaches the `exit` event
   handlers.

2. Replace `var`s with `let`s and `const`s.

3. Replace `==` based assertion with `strictEqual` assertion.

4. Use `common.PORT` instead of `5959`.

5. The test used to expect only one string "connecting to
   localhost:5959 ... ok", but the debugger actually emits another
   string, "break in test/fixtures/empty.js:2". This patch asserts if
   both of them are received in the same order.

Refer: nodejs#10361
PR-URL: nodejs#10455

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@thefourtheye thefourtheye mentioned this pull request Feb 5, 2017
Closed
2 tasks
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
@thefourtheye @MylesBorins
test: refactor test-debugger-remote
6966a58 
1. The test doesn't attach an event listener for `exit` events and
   removes them before killing. The intention is to fail the tests if
   the processes exit normally. This patch attaches the `exit` event
   handlers.

2. Replace `var`s with `let`s and `const`s.

3. Replace `==` based assertion with `strictEqual` assertion.

4. Use `common.PORT` instead of `5959`.

5. The test used to expect only one string "connecting to
   localhost:5959 ... ok", but the debugger actually emits another
   string, "break in test/fixtures/empty.js:2". This patch asserts if
   both of them are received in the same order.

Refer: #10361
PR-URL: #10455

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@thefourtheye @MylesBorins
test: refactor test-debugger-remote
ae338da 
1. The test doesn't attach an event listener for `exit` events and
   removes them before killing. The intention is to fail the tests if
   the processes exit normally. This patch attaches the `exit` event
   handlers.

2. Replace `var`s with `let`s and `const`s.

3. Replace `==` based assertion with `strictEqual` assertion.

4. Use `common.PORT` instead of `5959`.

5. The test used to expect only one string "connecting to
   localhost:5959 ... ok", but the debugger actually emits another
   string, "break in test/fixtures/empty.js:2". This patch asserts if
   both of them are received in the same order.

Refer: #10361
PR-URL: #10455

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

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
@thefourtheye @MylesBorins @jasnell @targos @mscdex @nodejs-github-bot