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: remove bluebird remnants from test fixture #31435

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 21, 2020

The test fixture in test/fixtures/bluebird was largely copied from
bluebird, where a regression in Node.js was discovered. Simplify the
test by removing a lot of things that aren't necessary to replicate the
problem. Change name from bluebird to something less likely to cause
someone to believe that we are actually loading bluebird (as we are
not).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 21, 2020
@Trott Trott changed the title test: de-bluebird-ize test fixture test: remove bluebird remnants from test fixture Jan 21, 2020
@Trott Trott requested a review from richardlau January 21, 2020 05:46
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

LGTM. Left an observation.

@mscdex
Copy link
Contributor

mscdex commented Jan 21, 2020

I'm not sure that 'fhqwhgads' is a better name. Perhaps something more descriptive would be better...

@Trott
Copy link
Member Author

Trott commented Jan 21, 2020

I'm not sure that 'fhqwhgads' is a better name. Perhaps something more descriptive would be better...

Open to suggestions if we can make it reasonably concise but still meaningful. enoent-main-with-valid-copy-in-node-modules is about twice as long as I'd like.

@@ -0,0 +1,4 @@
{
"name": "fhqwhgads",
"main": "./js/release/fhqwhgads.js"
Copy link
Member

Choose a reason for hiding this comment

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

We could use package-name instead of the random characters.

@Trott
Copy link
Member Author

Trott commented Jan 21, 2020

@mscdex @BridgeAR I changed the name to package-main-enoent and added a comment that explains more fully.

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@aks- aks- left a comment

Choose a reason for hiding this comment

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

😍

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 22, 2020
The test fixture in test/fixtures/bluebird was largely copied from
bluebird, where a regression in Node.js was discovered. Simplify the
test by removing a lot of things that aren't necessary to replicate the
problem. Change name from bluebird to something less likely to cause
someone to believe that we are actually loading bluebird (as we are
not).

PR-URL: nodejs#31435
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jan 23, 2020

Landed in dc90f92

@Trott Trott merged commit dc90f92 into nodejs:master Jan 23, 2020
@Trott Trott deleted the de-bluebird branch January 23, 2020 06:11
codebytere pushed a commit that referenced this pull request Feb 17, 2020
The test fixture in test/fixtures/bluebird was largely copied from
bluebird, where a regression in Node.js was discovered. Simplify the
test by removing a lot of things that aren't necessary to replicate the
problem. Change name from bluebird to something less likely to cause
someone to believe that we are actually loading bluebird (as we are
not).

PR-URL: #31435
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
The test fixture in test/fixtures/bluebird was largely copied from
bluebird, where a regression in Node.js was discovered. Simplify the
test by removing a lot of things that aren't necessary to replicate the
problem. Change name from bluebird to something less likely to cause
someone to believe that we are actually loading bluebird (as we are
not).

PR-URL: #31435
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: David Carlier <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
The test fixture in test/fixtures/bluebird was largely copied from
bluebird, where a regression in Node.js was discovered. Simplify the
test by removing a lot of things that aren't necessary to replicate the
problem. Change name from bluebird to something less likely to cause
someone to believe that we are actually loading bluebird (as we are
not).

PR-URL: #31435
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants