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: add batch of known issue tests #5653

Merged
merged 1 commit into from
Mar 12, 2016
Merged

test: add batch of known issue tests #5653

merged 1 commit into from
Mar 12, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 11, 2016

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][0]?
  • 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)?

Affected core subsystem(s)

test

Description of change

This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734

R=@Trott

@mscdex mscdex added test Issues and PRs related to the tests. known issue test labels Mar 11, 2016
@mscdex
Copy link
Contributor

mscdex commented Mar 11, 2016

FWIW there is already a PR for the known test for #728 in #5649

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

LGTM sans the duplicated #728 test.

@Trott
Copy link
Member

Trott commented Mar 11, 2016

LGTM if CI is happy (which means the tests pass linting). I don't mind going with this test for #728 and closing the PR I opened for that.

@Trott
Copy link
Member

Trott commented Mar 11, 2016

All of these tests appear to use the wrong path for common. Should be ../common rather than ./test/common.

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 11, 2016

Oops, I had been working on the tests in a different directory. Fixed now.

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 11, 2016

And make jslint passes.

@Trott
Copy link
Member

Trott commented Mar 11, 2016

I guess make test-known-issues passes too? Tangent: Should test and/or test-ci include test-known-issues so that we make sure we remove tests from there when the issues are fixed (especially if we fix them accidentally)?

@Trott
Copy link
Member

Trott commented Mar 11, 2016

(And, of course: Still LGTM!)

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 12, 2016

Should test and/or test-ci include test-known-issues so that we make sure we remove tests from there when the issues are fixed (especially if we fix them accidentally)?

I'm leaning toward no. I wanted this functionality mostly to help triage the issue tracker. I created the repro-exists label to add to known issues that have tests. If we accidentally fix something, I'd hate to see it cause any friction at all with a legitimate PR. That said, if others wanted to add this to test or test-ci, I wouldn't fight over it.

This commit adds tests for several known issues.

Refs: nodejs#1901
Refs: nodejs#728
Refs: nodejs#4778
Refs: nodejs#947
Refs: nodejs#2734
PR-URL: nodejs#5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 12, 2016

Landed in 10bc673. Thanks for the reviews.

@jasnell
Copy link
Member

jasnell commented Mar 12, 2016

Marking this lts-watch but it depends on landing the original PR that adds this capability (can't recall the pr number off hand)

@rvagg
Copy link
Member

rvagg commented Mar 14, 2016

+1 for LTS as with the original addition of this dir

evanlucas pushed a commit that referenced this pull request Mar 14, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 14, 2016
rvagg pushed a commit that referenced this pull request Mar 16, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins
Copy link
Contributor

@jasnell the functionality was backported in #5785

landing into staging now

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants