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: test sys rather than common #3256

Closed
wants to merge 3 commits into from
Closed

test: test sys rather than common #3256

wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 8, 2015

test-sys.js seems to test common.inspect() (which is test-specific code) and not sys (which, although deprecated, should still be tested).

This seems like it was introduced in 9fd5e3c which is a pretty big commit with lots of probable find/replace.

This commit changes it to test sys which is what the file name implies it should be doing.

@Trott Trott added the test Issues and PRs related to the tests. label Oct 8, 2015
@Trott Trott mentioned this pull request Oct 8, 2015
Closed
@targos
Copy link
Member

targos commented Oct 8, 2015

Since sys is just the same as util, can't we just make sure that all the tests in this file are done in the test-util* files and then just remove everything and do assert.strictEqual(require('sys'), require('util')) ?

@evanlucas
Copy link
Contributor

I'm with @targos on this. A lot of the tests are duplicated anyways

@Trott
Copy link
Member Author

Trott commented Oct 9, 2015

OK, done, all sys tests moved to util and sys test replaced with a strictEqual() check that sys and util are the same thing.

@evanlucas
Copy link
Contributor

LGTM

@Trott
Copy link
Member Author

Trott commented Oct 9, 2015

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

Assuming CI is green, I'll land this in the next few hours unless someone objects.

@Trott
Copy link
Member Author

Trott commented Oct 9, 2015

Well, that was embarrassing. Let's try this again. CI: https://ci.nodejs.org/job/node-test-pull-request/467/

@Trott
Copy link
Member Author

Trott commented Oct 9, 2015

@evanlucas Very minor change to correct a test I botched. Still LGTY? /cc @targos

@evanlucas
Copy link
Contributor

yep, LGTM

Trott added a commit that referenced this pull request Oct 9, 2015
@Trott
test: test util rather than common
ead5cd9 
test-sys.js tests common.inspect() (which is test-specific code) and not
sys (which, although deprecated, should still be tested).

This commit moves the tests to the not-deprecated util and adds a test
to check that deprecated sys and util are the same.

PR-URL: #3256
Reviewed-By: Evan Lucas <evanlucas@me.com>
@Trott
Copy link
Member Author

Trott commented Oct 9, 2015

Landed in ead5cd9

@Trott Trott closed this Oct 9, 2015
Trott added a commit that referenced this pull request Oct 10, 2015
@Trott @jasnell
test: test util rather than common
5ca4f6f 
test-sys.js tests common.inspect() (which is test-specific code) and not
sys (which, although deprecated, should still be tested).

This commit moves the tests to the not-deprecated util and adds a test
to check that deprecated sys and util are the same.

PR-URL: #3256
Reviewed-By: Evan Lucas <evanlucas@me.com>
@jasnell
Copy link
Member

jasnell commented Oct 10, 2015

Landed in v4.x in 5ca4f6f

@jasnell jasnell mentioned this pull request Oct 10, 2015
Closed
@Trott Trott deleted the test-util branch January 13, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
5 participants
@Trott @targos @evanlucas @jasnell @MylesBorins