Skip to content

test: add batch of known issue tests #5653

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
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.
@Trott Trott mentioned this pull request Mar 11, 2016
Closed
2 tasks
@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.
@cjihrig
test: add batch of known issue tests
10bc673 
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 <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig cjihrig merged commit 10bc673 into nodejs:master Mar 12, 2016
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 12, 2016

Landed in 10bc673. Thanks for the reviews.
@cjihrig cjihrig deleted the bugs branch March 12, 2016 14:58
@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 evanlucas mentioned this pull request Mar 14, 2016
Closed
evanlucas pushed a commit that referenced this pull request Mar 14, 2016
@cjihrig @evanlucas
test: add batch of known issue tests
c783dd4 
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 <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
@evanlucas evanlucas mentioned this pull request Mar 14, 2016
Merged
rvagg pushed a commit that referenced this pull request Mar 16, 2016
@cjihrig @rvagg
test: add batch of known issue tests
9a44c8c 
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 <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@cjihrig
test: add batch of known issue tests
cc2690d 
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 <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 30, 2016

@jasnell the functionality was backported in #5785

landing into staging now
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@cjihrig
test: add batch of known issue tests
78effc3 
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 <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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.

6 participants

@cjihrig @mscdex @jasnell @Trott @rvagg @MylesBorins