search

typesafehub / npm

Other
3 stars 6 forks source link

Added test for jsdom #5

Closed jroper closed 7 years ago

benmccann commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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.