Skip to content

Conversation

@Trott
Copy link
Member

@Trott Trott commented May 27, 2016

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

test

Description of change

Previous version of weak used for gc tests emitted a warning on OS X.
Updating to current version eliminates warning.

R=@bnoordhuis?

Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning.
@Trott Trott added the test Issues and PRs related to the tests. label May 27, 2016
@bnoordhuis
Copy link
Member

I'm not about to review a +9,391 −279 diff so rubber-stamp LGTM.

Meta: The gc tests are not run regularly. How about we either add them to the CI or we scrap them? I personally don't find them that valuable but I'm interested in what others think.

@jasnell
Copy link
Member

jasnell commented May 27, 2016

Oy, yeah, that diff is harsh lol. rubber-stamp LGTM here also if CI is possible. Also agree with @bnoordhuis in that if we're not running these regularly, they're not of very much use to keep around.

@Fishrock123
Copy link
Contributor

Maybe we should run these in citgm? cc @thealphanerd ?

@Trott
Copy link
Member Author

Trott commented May 27, 2016

@Fishrock123 The changes are in test/gc/node_modules. I may be mistaken, but I don't think CITGM would be affected by that or exercise that code at all. In fact, I'm pretty sure nothing is affected except people (like me) who sometimes run make test-gc at the command line.

@Trott
Copy link
Member Author

Trott commented May 27, 2016

@Fishrock123 Oh, wait, maybe you're just suggesting that weak be added to the list of modules that CITGM tests? If so: Yes, +1 to that.

@Fishrock123
Copy link
Contributor

@Trott I'm suggesting that maybe we should put it in a separate repo and include it in the citgm tests.

@Trott
Copy link
Member Author

Trott commented May 31, 2016

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

EDIT: One Pi buildbot explosion and one Pi inexplicably hung during a git operation. Otherwise, all green in CI.

Trott added a commit to Trott/io.js that referenced this pull request May 31, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: nodejs#7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented May 31, 2016

Landed in 0d7f313

@Trott Trott closed this May 31, 2016
Fishrock123 pushed a commit that referenced this pull request Jun 1, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Previous version of weak used for gc tests emitted a warning on OS X. Updating to current version eliminates warning. PR-URL: #7014 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the weak 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.

5 participants