Skip to content

test: add a place for reproducing known bugs - #5528

Closed
cjihrig wants to merge 2 commits into
nodejs:masterfrom
cjihrig:bugs
Closed

test: add a place for reproducing known bugs#5528
cjihrig wants to merge 2 commits into
nodejs:masterfrom
cjihrig:bugs

Conversation

@cjihrig

@cjihrigcjihrig commented Mar 2, 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)?

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

Affected core subsystem(s)

test

Description of change

This PR is a proof of concept related to nodejs/testing#18. The point is to create a place to track the state of current known bugs, with a single command to verify that they still exist (the issue tracker has proven to be a difficult way to manage this).

There are currently a few hundred open issues, so I only bothered to port one bug reproduction. I also need to add make test-bugs to vcbuild for Windows. If this can get in, I'll work to create more reproductions.

@jasnell

jasnell commented Mar 2, 2016

Copy link
Copy Markdown
Member

Oh nice... I have some concerns about maintainability of this but generally LGTM

@jasnelljasnell added the test Issues and PRs related to the tests. label Mar 2, 2016
geek
geek reviewed Mar 2, 2016
View reviewed changes
Comment threadMakefile Outdated

@geekgeekMar 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead of bugs, maybe name the folder and make step: broken

@TrottTrottMar 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To me, broken makes it sound like a place where we put tests that are broken, rather than a place where we have good tests that are failing because of bugs in the main code base. I prefer test-bugs or even test-known-issues.

@cjihrigcjihrigMar 2, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I like test-known-issues. @geek are you cool with that?

@geekgeekMar 3, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@cjihrig agreed

@geek

geek commented Mar 3, 2016

Copy link
Copy Markdown
Member

LGTM

@cjihrig
tools: support testing known issues
29ba2eb
This commit adds a known_issues directory to the test directory
for scripts that reproduce known bugs. Since these scripts are
expected to fail, it also adds a --expect-fail flag to test.py
which reports tests as successful when they fail.
@cjihrig
test: bug repro for vm function redefinition
48e7009
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.
Refs: nodejs#548
@cjihrig

cjihrig commented Mar 3, 2016

Copy link
Copy Markdown
ContributorAuthor

Added the vcbuild piece.

@Trott care to sign off on this?

EDIT: Also renamed bugs to known-issues.

@Trott

Trott commented Mar 3, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green.

@cjihrig

cjihrig commented Mar 3, 2016

Copy link
Copy Markdown
ContributorAuthor

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

@cjihrig

cjihrig commented Mar 3, 2016

Copy link
Copy Markdown
ContributorAuthor

Only failure is the ARM machine that appeared to be offline for several a number of CI runs.

cjihrig added a commit that referenced this pull request Mar 4, 2016
@cjihrig
tools: support testing known issues
32e1f9d
This commit adds a known_issues directory to the test directory
for scripts that reproduce known bugs. Since these scripts are
expected to fail, it also adds a --expect-fail flag to test.py
which reports tests as successful when they fail.
Refs: nodejs/testing#18
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
cjihrig added a commit that referenced this pull request Mar 4, 2016
@cjihrig
test: bug repro for vm function redefinition
e38e2a6
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.
Refs: #548
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig

cjihrig commented Mar 4, 2016

Copy link
Copy Markdown
ContributorAuthor

Thanks for the reviews. Landed in 32e1f9d and e38e2a6.

@cjihrigcjihrig closed this Mar 4, 2016
@cjihrig
cjihrig deleted the bugs branch March 4, 2016 13:55
@cjihrigcjihrig mentioned this pull request Mar 4, 2016
Closed
@TrottTrott mentioned this pull request Mar 4, 2016
Closed
4 tasks
@Fishrock123Fishrock123 mentioned this pull request Mar 7, 2016
Merged
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
@cjihrig@Fishrock123
tools: support testing known issues
694aa91
This commit adds a known_issues directory to the test directory
for scripts that reproduce known bugs. Since these scripts are
expected to fail, it also adds a --expect-fail flag to test.py
which reports tests as successful when they fail.
Refs: nodejs/testing#18
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
@cjihrig@Fishrock123
test: bug repro for vm function redefinition
c8b6e94
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.
Refs: #548
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
@cjihrig@Fishrock123
tools: support testing known issues
5aa17dc
This commit adds a known_issues directory to the test directory
for scripts that reproduce known bugs. Since these scripts are
expected to fail, it also adds a --expect-fail flag to test.py
which reports tests as successful when they fail.
Refs: nodejs/testing#18
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
@cjihrig@Fishrock123
test: bug repro for vm function redefinition
29e26b3
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.
Refs: #548
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 10, 2016
Closed
@MylesBorins

MylesBorins commented Mar 10, 2016

Copy link
Copy Markdown
Contributor

This seems like the kind of thing useful to backport. @cjihrig do you agree?

@cjihrig

cjihrig commented Mar 11, 2016

Copy link
Copy Markdown
ContributorAuthor

I think it would be useful, but could be tricky to manage.

@jasnell

jasnell commented Mar 11, 2016

Copy link
Copy Markdown
Member

hmm... not so sure about this one. @nodejs/lts ... thoughts?

@Trott

Trott commented Mar 11, 2016

Copy link
Copy Markdown
Member

I would think you would want to put this in LTS. You're going to want the bug fixes for these things in LTS and not having the tests there just increases the friction when you go to land the bug fixes. It's just a directory of tests that don't get run.

@jasnell

jasnell commented Mar 11, 2016

Copy link
Copy Markdown
Member

Agreed in principle, but there will be a bit of a maintenance burden as things get fixed in one branch but not others. I do see how it's useful tho and definitely wouldn't -1 it

@rvagg

rvagg commented Mar 14, 2016

Copy link
Copy Markdown
Member

I think it's a +1 from me, reflecting on the difficulty we have with the 0.10 and 0.12 test suite, what we have on master is only going to get better and having this in v4 will help it as we get toward EOL to understand more about the delta and the state of the code. I also imagine that there will be a section of users who will be interested in this kind of information on v4 when contributing because that's the only branch they care about (we see this already on 0.10 and 0.12).

@MylesBorins

MylesBorins commented Mar 17, 2016

Copy link
Copy Markdown
Contributor

so this does not land cleanly on v4.x-staging@cjihrig would you be able to send a PR directly against v4.x-staging with the conflicts sorted out? They were primarily in test.py

cjihrig added a commit to cjihrig/node that referenced this pull request Mar 18, 2016
@cjihrig
tools: support testing known issues
4b04cb4
This commit adds a known_issues directory to the test directory
for scripts that reproduce known bugs. Since these scripts are
expected to fail, it also adds a --expect-fail flag to test.py
which reports tests as successful when they fail.
Refs: nodejs/testing#18
PR-URL: nodejs#5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Conflicts:
tools/test.py
cjihrig added a commit to cjihrig/node that referenced this pull request Mar 18, 2016
@cjihrig
test: bug repro for vm function redefinition
bab9aaf
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.
Refs: nodejs#548
PR-URL: nodejs#5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrigcjihrig mentioned this pull request Mar 18, 2016
Closed
@cjihrig

cjihrig commented Mar 18, 2016

Copy link
Copy Markdown
ContributorAuthor

Backport in #5785

MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@cjihrigMyles Borins
tools: support testing known issues
1735260
This commit adds a known_issues directory to the test directory
for scripts that reproduce known bugs. Since these scripts are
expected to fail, it also adds a --expect-fail flag to test.py
which reports tests as successful when they fail.
Refs: nodejs/testing#18
Backport-URL: #5785
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Conflicts:
tools/test.py
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@cjihrigMyles Borins
test: bug repro for vm function redefinition
7725cd3
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.
Refs: #548
Backport-URL: #5785
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@cjihrigMyles Borins
tools: support testing known issues
cda345e
This commit adds a known_issues directory to the test directory
for scripts that reproduce known bugs. Since these scripts are
expected to fail, it also adds a --expect-fail flag to test.py
which reports tests as successful when they fail.
Refs: nodejs/testing#18
Backport-URL: #5785
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Conflicts:
tools/test.py
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@cjihrigMyles Borins
test: bug repro for vm function redefinition
3e32397
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.
Refs: #548
Backport-URL: #5785
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@cjihrigMyles Borins
tools: support testing known issues
7501ddc
This commit adds a known_issues directory to the test directory
for scripts that reproduce known bugs. Since these scripts are
expected to fail, it also adds a --expect-fail flag to test.py
which reports tests as successful when they fail.
Refs: nodejs/testing#18
Backport-URL: #5785
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Conflicts:
tools/test.py
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@cjihrigMyles Borins
test: bug repro for vm function redefinition
9952bcf
This commit adds a failing test case for the vm module.
Currently, if runInContext() defines a function, and a later call
to runInContext() redefines the same function, the original
function is not overwritten.
Refs: #548
Backport-URL: #5785
PR-URL: #5528
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cjihrig@jasnell@geek@Trott@MylesBorins@rvagg