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: move uncoupled debugger tests to sequential #6731

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 13, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test debugger

Description of change

Move tests from test/debugger to test/sequential so that they are
exercised by CI and make test. This change only moves tests that have
no dependencies that break when moved to a different directory.

The remaining tests can move in a subsequent PR that will have other (minor) changes required so the tests can run in sequential.

/cc @bnoordhuis @santigimeno

@Trott Trott added debugger test Issues and PRs related to the tests. labels May 13, 2016
@santigimeno
Copy link
Member

LGTM if CI is green: https://ci.nodejs.org/job/node-test-pull-request/2628/

@cjihrig
Copy link
Contributor

cjihrig commented May 13, 2016

LGTM in theory. The CI seemed to have a bad time though.

@Trott
Copy link
Member Author

Trott commented May 16, 2016

Ugh. I think I see what the problem is, but uh, small steps first. It looks like one of the tests moved is working A-OK in CI so let's move that one first... moved the others back and force pushed...

CI: https://ci.nodejs.org/job/node-test-commit/3350/

Move test from `test/debugger` to `test/sequential` so that it is
exercised by CI and `make test`.
@Trott
Copy link
Member Author

Trott commented May 16, 2016

(Amended commit message and force pushed.)

@Trott
Copy link
Member Author

Trott commented May 16, 2016

CI is green.

@jasnell
Copy link
Member

jasnell commented May 16, 2016

LGTM but I'm wondering if we should hold off a bit to see what's causing the others to fail.

@Trott
Copy link
Member Author

Trott commented May 16, 2016

@jasnell I think the others are failing because of a race condition but I'd like to address it separately. Since these tests have not been seeing any activity with any regularity, I guess it's not surprising that they may not be 100% bullet-proof.

I'd prefer to move the one working test over by itself so that if something comes up and we do have to revert something somewhere, it's an easy and atomic revert that doesn't affect multiple things at once. The other tests will need to be modified and I'd rather blend the move-as-is commit with those modifications.

@jasnell
Copy link
Member

jasnell commented May 16, 2016

Ok. No problems then
On May 16, 2016 1:07 PM, "Rich Trott" notifications@github.com wrote:

@jasnell https://github.com/jasnell I think the others are failing
because of a race condition but I'd like to address it separately. Since
these tests have not been seeing any activity with any regularity, I guess
it's not surprising that they may not be 100% bullet-proof.

I'd prefer to move the one working test over by itself so that if
something comes up and we do have to revert something somewhere, it's an
easy and atomic revert that doesn't affect multiple things at once. The
other tests will need to be modified and I'd rather not junk up the
move-as-is commit with those modifications.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6731 (comment)

Trott added a commit to Trott/io.js that referenced this pull request May 16, 2016
Move test from `test/debugger` to `test/sequential` so that it is
exercised by CI and `make test`.

PR-URL: nodejs#6731
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented May 16, 2016

Landed in 5a5b74a

@Trott Trott closed this May 16, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
Move test from `test/debugger` to `test/sequential` so that it is
exercised by CI and `make test`.

PR-URL: #6731
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the move-debug branch January 13, 2022 22:43
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.

5 participants