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: clean up repl-reset-event file #9931

Closed
wants to merge 1 commit into from

Conversation

courtnek
Copy link
Contributor

@courtnek courtnek commented Dec 1, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change
  • Change vars to let/const
  • Add mustCall
  • equal -> strictEqual
  • remove timeout

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Dec 1, 2016
', and is not using global as context');
assert.strictEqual(context, r.context, 'REPL emitted incorrect context');
assert.strictEqual(context.foo, undefined, 'REPL emitted the previous' +
' context, and is not using global as context');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you line this up with the previous line.

@Trott
Copy link
Member

Trott commented Dec 10, 2016

Ping @courtnek: Thanks for the work on this! Can you make the change requested by @cjihrig and push it to this branch?

If you want, you can copy/paste this into the file:

'use strict';
const common = require('../common');
common.globalCheck = false;

const assert = require('assert');
const repl = require('repl');

// Create a dummy stream that does nothing
const dummy = new common.ArrayStream();

function testReset(cb) {
  const r = repl.start({
    input: dummy,
    output: dummy,
    useGlobal: false
  });
  r.context.foo = 42;
  r.on('reset', common.mustCall(function(context) {
    assert(!!context, 'REPL did not emit a context with reset event');
    assert.strictEqual(context, r.context, 'REPL emitted incorrect context');
    assert.strictEqual(
      context.foo,
      undefined,
      'REPL emitted the previous context, and is not using global as context'
    );
    context.foo = 42;
    cb();
  }));
  r.resetContext();
}

function testResetGlobal() {
  const r = repl.start({
    input: dummy,
    output: dummy,
    useGlobal: true
  });
  r.context.foo = 42;
  r.on('reset', common.mustCall(function(context) {
    assert.strictEqual(
      context.foo,
      42,
      '"foo" property is missing from REPL using global as context'
    );
  }));
  r.resetContext();
}

testReset(common.mustCall(testResetGlobal));

@courtnek
Copy link
Contributor Author

Hey @cjihrig @Trott,

Sorry for the delay!

Yeah @Trott I like breaking up the function args over multiple lines over splitting the string. I amended my commit with the new style.

Let me know if there is anything else I can do.

* Change vars to let/const
* Add mustCall
* equal -> strictEqual
* remove timeout
@Trott
Copy link
Member

Trott commented Dec 21, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/5519/

@cjihrig Looks like your comment has been addressed. Looks good to you now?

@Trott
Copy link
Member

Trott commented Dec 22, 2016

CI is ✅

@jasnell
Copy link
Member

jasnell commented Dec 24, 2016

ping @cjihrig

jasnell pushed a commit that referenced this pull request Dec 24, 2016
* Change vars to let/const
* Add mustCall
* equal -> strictEqual
* remove timeout

PR-URL: #9931
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Dec 24, 2016

Landed in e81e031

@jasnell jasnell closed this Dec 24, 2016
@Trott
Copy link
Member

Trott commented Dec 24, 2016

Thanks for the contribution, @courtnek! 🎉

targos pushed a commit that referenced this pull request Dec 26, 2016
* Change vars to let/const
* Add mustCall
* equal -> strictEqual
* remove timeout

PR-URL: #9931
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Dec 26, 2016
* Change vars to let/const
* Add mustCall
* equal -> strictEqual
* remove timeout

PR-URL: #9931
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Dec 27, 2016
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
* Change vars to let/const
* Add mustCall
* equal -> strictEqual
* remove timeout

PR-URL: #9931
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
* Change vars to let/const
* Add mustCall
* equal -> strictEqual
* remove timeout

PR-URL: #9931
Reviewed-By: James M Snell <[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
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants