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: refactor test-repl-sigint #11309

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@Trott
Member

Trott commented Feb 11, 2017

  • remove debugging code that prints child stdout
  • indexOf() -> includes()
  • improved messages on assertion failures
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test repl

test: refactor test-repl-sigint
* remove debugging code that prints child stdout
* indexOf() -> includes()
* improved messages on assertion failures

@Trott Trott added repl test labels Feb 11, 2017

@lpinca

lpinca approved these changes Feb 12, 2017

@thefourtheye

LGTM if CI is happy.

assert.notStrictEqual(stdout.indexOf('42042\n'), -1);
assert.ok(
stdout.includes('Script execution interrupted.\n'),
`Expected stdout to contain "Script execution interrupted.", got ${stdout}`

This comment has been minimized.

@thefourtheye

thefourtheye Feb 12, 2017

Contributor

Nit: \n is missing in the expected string.

@thefourtheye

thefourtheye Feb 12, 2017

Contributor

Nit: \n is missing in the expected string.

This comment has been minimized.

@Trott

Trott Feb 13, 2017

Member

I left it out because a literal \n in the message output would probably be unhelpful and an escaped \n might be taken to mean that an escaped \n is what is expected.

I'm OK with any of the three possibilities (leave it out, put it in literally, put it in escaped). I chose this one (leave it out) for simplicity. If you feel that it should be there, let me know if you want it escaped or not. And if, like me, you don't feel particularly strongly, then I'll probably just leave it as it is. ¯\(ツ)

@Trott

Trott Feb 13, 2017

Member

I left it out because a literal \n in the message output would probably be unhelpful and an escaped \n might be taken to mean that an escaped \n is what is expected.

I'm OK with any of the three possibilities (leave it out, put it in literally, put it in escaped). I chose this one (leave it out) for simplicity. If you feel that it should be there, let me know if you want it escaped or not. And if, like me, you don't feel particularly strongly, then I'll probably just leave it as it is. ¯\(ツ)

@Trott

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Feb 13, 2017

test: refactor test-repl-sigint
* remove debugging code that prints child stdout
* indexOf() -> includes()
* improved messages on assertion failures

PR-URL: #11309
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 13, 2017

Member

Landed in f2023d7

Member

jasnell commented Feb 13, 2017

Landed in f2023d7

@jasnell jasnell closed this Feb 13, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: refactor test-repl-sigint
* remove debugging code that prints child stdout
* indexOf() -> includes()
* improved messages on assertion failures

PR-URL: nodejs#11309
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

test: refactor test-repl-sigint
* remove debugging code that prints child stdout
* indexOf() -> includes()
* improved messages on assertion failures

PR-URL: #11309
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

Needs a backport PR to land on v4

Member

jasnell commented Mar 7, 2017

Needs a backport PR to land on v4

MylesBorins added a commit that referenced this pull request Mar 9, 2017

test: refactor test-repl-sigint
* remove debugging code that prints child stdout
* indexOf() -> includes()
* improved messages on assertion failures

PR-URL: #11309
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment