FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add batch of known issue tests by cjihrig · Pull Request #5653 · nodejs/node · GitHub

/ node Public

test: add batch of known issue tests - #5653

Merged
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:bugs
Mar 12, 2016
ConversationCommits (1)ChecksFiles changed
Merged

test: add batch of known issue tests#5653
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:bugs

Conversation

cjihrig commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

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 added test Issues and PRs related to the tests. known issue test labels Mar 11, 2016

mscdex commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

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

jasnell commented Mar 11, 2016

Copy link
Copy Markdown
Member

LGTM sans the duplicated #728 test.

Trott commented Mar 11, 2016

Copy link
Copy Markdown
Member

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 commented Mar 11, 2016

Copy link
Copy Markdown
Member

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

Trott mentioned this pull request Mar 11, 2016
Closed
2 tasks

cjihrig commented Mar 11, 2016

Copy link
Copy Markdown
Contributor Author

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

cjihrig commented Mar 11, 2016

Copy link
Copy Markdown
Contributor Author

And make jslint passes.

Trott commented Mar 11, 2016

Copy link
Copy Markdown
Member

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 commented Mar 11, 2016

Copy link
Copy Markdown
Member

(And, of course: Still LGTM!)

cjihrig commented Mar 12, 2016

Copy link
Copy Markdown
Contributor Author

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.

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 merged commit 10bc673 into nodejs:master Mar 12, 2016

cjihrig commented Mar 12, 2016

Copy link
Copy Markdown
Contributor Author

Landed in 10bc673. Thanks for the reviews.

cjihrig deleted the bugs branch March 12, 2016 14:58

jasnell commented Mar 12, 2016

Copy link
Copy Markdown
Member

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 commented Mar 14, 2016

Copy link
Copy Markdown
Member

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

evanlucas mentioned this pull request Mar 14, 2016
Closed
evanlucas pushed a commit that referenced this pull request Mar 14, 2016
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 mentioned this pull request Mar 14, 2016
Merged
rvagg pushed a commit that referenced this pull request Mar 16, 2016
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
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 commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

@jasnell the functionality was backported in #5785

landing into staging now

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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.

6 participants

Back | FazBrowse Home | New Git URL