Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Fix nodereport requires #11

Merged
merged 2 commits into from
Nov 1, 2016
Merged

Fix nodereport requires #11

merged 2 commits into from
Nov 1, 2016

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Oct 28, 2016

The tests would only run when nodereport was itself git cloned into a directory called node_modules, tricking the require system into thinking nodereport was a dependency of itself.

Partially fixes #7

A module X can only require a module Y if it depends on it in its
package.json and its installed. nodereport cannot depend on itself, so
it cannot require itself by name, it has to require itself by path.
@sam-github sam-github mentioned this pull request Oct 28, 2016
@rnchamberlain
Copy link
Contributor

I tried it and it works for me, LGTM

@sam-github
Copy link
Contributor Author

@rnchamberlain as one of the core collaborators, you can press the merge button as soon as you have LGTMed, and its sat for 48 hours for someone else to say 👎 if they want. I don't have merge auth.

@mhdawson
Copy link
Member

mhdawson commented Nov 1, 2016

LGTM

@rnchamberlain rnchamberlain merged commit ba89b25 into nodejs:master Nov 1, 2016
@rnchamberlain
Copy link
Contributor

@sam-github one more question on this: how do we run the tests against an 'npm installed' nodereport module? I think that's a reasonable scenario. So we get the tests now by cloning the repo from github, but the relative path require() only works if we 'node-gyp build' that cloned nodereport. One way to test the original npm install is to copy -R the tests folder from the git clone across to node_modules/nodereport, but that seems clunky to me. Thoughts?

@sam-github
Copy link
Contributor Author

I think that's a reasonable scenario.

Why? What are you testing? npm is for delivering requireable dependencies, not for downloading project source for development. This seems to me to be a test that when you npm publish/install, the output files are the same as the input files. Its a test of npm-the-tool, not a test of the package.

When you get a python module, does it come with tests? What about a java jar file or compiled C library? They don't come with tests either, the tests are run on the built artifacts before pushing them into whatever packaging system is used to distribute the artifacts.

Or take node, I don't think you can run its tests against the node installed in the system, though maybe there is some hack I'm unaware of.

@sam-github sam-github deleted the fix-nodereport-requires branch November 7, 2016 21:24
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.

tests all fail
4 participants