Skip to content
This repository has been archived by the owner on Apr 11, 2022. It is now read-only.

Added test for jsdom #5

Merged
merged 1 commit into from Feb 23, 2017
Merged

Added test for jsdom #5

merged 1 commit into from Feb 23, 2017

Conversation

jroper
Copy link

@jroper jroper commented Jun 22, 2015

No description provided.

@wsargent wsargent merged commit a721662 into typesafehub:master Feb 23, 2017
@benmccann
Copy link

@jroper the build is unfortunately failing since @wsargent merged this PR. Do you remember why stdOut was supposed to containnode-gyp rebuild or what problem it indicated if that was not present?

I'm wondering if we should revert this commit. I spent awhile messing with this and couldn't figure out how to get the stdOut to contain node-gyp rebuild

@jroper jroper deleted the jsdom branch March 5, 2017 23:29
@jroper
Copy link
Author

jroper commented Mar 5, 2017

node-gyp is the node native compilation tool. From memory, if you add jsdom to a build, which contains a native portion, it should trigger a node-gyp compilation, and I added this test because support for it is quite fragile, and so I thought it valuable to add a test that was able to detect if it regressed. So reverting is definitely not the solution, since there's a high chance that a regression has occurred. Another possibility is that npm now outputs different log messages when it does a node-gyp compilation, in which case the test should be updated.

So the first thing to do is check whether jsdom still works, if it does then there's no regression, instead we should just update the test to ensure that node-gyp still works.

@jroper
Copy link
Author

jroper commented Mar 6, 2017

The problem appears to be that npm introduced an optimisation where node-gyp doesn't need to be rerun (either that, or this test only ever worked on a clean checkout of the repository). The solution is to clean up the node_modules directory before running the test.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants