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: reduce brittleness of tab complete test #5772

Closed
wants to merge 1 commit into from
Closed

test: reduce brittleness of tab complete test #5772

wants to merge 1 commit into from

Conversation

matthewloring
Copy link

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

test

Description of change

Please provide a description of the change here.

test-repl-tab-complete includes tests that ensure that certain keys do
not appear in tab completions or that completion does not crash the
repl. It performed these tests by comparing completion output to a
hardcoded list of expected keys. This list is made incorrect by any api
changes that occur when new versions of V8 are introduced.

With this change, we assert that the specific keys to be avoided are
not present in the output instead.

@claudiorodriguez claudiorodriguez added repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests. labels Mar 18, 2016
putIn.run(['.clear']);
putIn.run(['var obj = {1:"a","1a":"b",a:"b"};']);

testMe.complete('obj.', common.mustCall(function(error, data) {
assert.deepEqual(data, obj_elements);
assert.equal(data[0].indexOf('obj.1'), -1);
assert.equal(data[0].indexOf('obj.1a'), -1);
}));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe also check that obj.a is there.

Copy link
Author

Choose a reason for hiding this comment

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

Definitely, test added.

@Trott
Copy link
Member

Trott commented Mar 18, 2016

LGTM if CI is green. Left a nit that you can ignore if you want.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2016

I'd prefer strict equality checks in the test, but other than that, LGTM pending CI.

test-repl-tab-complete includes tests that ensure that certain keys do
not appear in tab completions or that completion does not crash the
repl. It performed these tests by comparing completion output to a
hardcoded list of expected keys. This list is made incorrect by any api
changes that occur when new versions of V8 are introduced.

With this change, we assert that the specific keys to be avoided are
not present in the output instead.
@matthewloring
Copy link
Author

@cjihrig Done.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2016

@matthewloring
Copy link
Author

Failures on smartos14-64 seem unrelated.

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

LGTM

@Trott
Copy link
Member

Trott commented Mar 18, 2016

Re-running CI for SmartOS: https://ci.nodejs.org/job/node-test-commit-smartos/1758/

@jasnell
Copy link
Member

jasnell commented Mar 19, 2016

CI is green

jasnell pushed a commit that referenced this pull request Mar 19, 2016
test-repl-tab-complete includes tests that ensure that certain keys do
not appear in tab completions or that completion does not crash the
repl. It performed these tests by comparing completion output to a
hardcoded list of expected keys. This list is made incorrect by any api
changes that occur when new versions of V8 are introduced.

With this change, we assert that the specific keys to be avoided are
not present in the output instead.

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

jasnell commented Mar 19, 2016

Landed in 0eb3ed5

@jasnell jasnell closed this Mar 19, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
test-repl-tab-complete includes tests that ensure that certain keys do
not appear in tab completions or that completion does not crash the
repl. It performed these tests by comparing completion output to a
hardcoded list of expected keys. This list is made incorrect by any api
changes that occur when new versions of V8 are introduced.

With this change, we assert that the specific keys to be avoided are
not present in the output instead.

PR-URL: #5772
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Conflicts:
	test/parallel/test-repl-tab-complete.js
@MylesBorins
Copy link
Contributor

This change relies on semver major changes... going to change to don't land for now, but open to it if someone wants to backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants