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 a place for reproducing known bugs #5528

Closed
wants to merge 2 commits into from

Conversation

cjihrig
Copy link
Contributor

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

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
Copy link
Member

jasnell commented Mar 2, 2016

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

@jasnell jasnell added the test Issues and PRs related to the tests. label Mar 2, 2016
@@ -192,6 +192,9 @@ test-internet: all
test-debugger: all
$(PYTHON) tools/test.py debugger

test-bugs: all
Copy link
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

Copy link
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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

@cjihrig agreed

@geek
Copy link
Member

geek commented Mar 3, 2016

LGTM

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.
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
Copy link
Contributor Author

cjihrig commented Mar 3, 2016

Added the vcbuild piece.

@Trott care to sign off on this?

EDIT: Also renamed bugs to known-issues.

@Trott
Copy link
Member

Trott commented Mar 3, 2016

LGTM if CI is green.

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 3, 2016

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 3, 2016

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
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
cjihrig added a commit that referenced this pull request Mar 4, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 4, 2016

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

@cjihrig cjihrig closed this Mar 4, 2016
@cjihrig cjihrig deleted the bugs branch March 4, 2016 13:55
@Fishrock123 Fishrock123 mentioned this pull request Mar 7, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins
Copy link
Contributor

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 11, 2016

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

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

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

@Trott
Copy link
Member

Trott commented Mar 11, 2016

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
Copy link
Member

jasnell commented Mar 11, 2016

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
Copy link
Member

rvagg commented Mar 14, 2016

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
Copy link
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
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Conflicts:
	tools/test.py
cjihrig added a commit to cjihrig/node that referenced this pull request Mar 18, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@cjihrig cjihrig mentioned this pull request Mar 18, 2016
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 18, 2016

Backport in #5785

MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Conflicts:
	tools/test.py
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Conflicts:
	tools/test.py
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Conflicts:
	tools/test.py
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
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 <[email protected]>
Reviewed-By: Wyatt Preul <[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