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

realpath for testing #30

Closed
FND opened this issue Apr 3, 2018 · 5 comments
Closed

realpath for testing #30

FND opened this issue Apr 3, 2018 · 5 comments

Comments

@FND
Copy link
Contributor

FND commented Apr 3, 2018

@mkhl noticed that various plugins use corutils' realpath, which is not available by default on macOS and various Linux distributions (cf. .travis.yml)

unfortunately, realpath adds another 46 packages (>1 MB) as dependencies, which seems silly:

let realpath = require("fs.realpath");

let filepath = sys.argv[2];
filepath = path.resolve(filepath);
filepath = realpath.realpathSync(filepath);
console.log(filepath);

thus we might want to offer a simplistic realpath implementation of our own - not quite sure how to do that safely though

@mkhl
Copy link
Contributor

mkhl commented Apr 3, 2018

I’m still not sure why realpath is required. I haven’t felt the need to symlink test/run anywhere yet…

@FND
Copy link
Contributor Author

FND commented Apr 3, 2018

I've become profoundly uncomfortable not ensuring absolute paths in cases like this:
https://github.com/faucet-pipeline/faucet-pipeline-js/blob/99c67e5dc3dcb82caf7d3245b4d7560417f8df25/test/cli/run#L5-L6

Is that excessively paranoid?

@mkhl
Copy link
Contributor

mkhl commented Apr 3, 2018

I believe it is.

Let’s assume we omit the call to realpath. In order for that to cause problems, someone would have to first symlink the script somewhere and then run it manually (npm test would still run it at the correct path). I believe that a failing script is the appropriate response to that scenario.

If someone were to actually do that, what problems would arise? Loading cli-harness.sh would fail and the script would abort with a helpful error message. If someone were to fix that error by fixing the path, the next begin would fail instead, again with a helpful error message.

I don’t think I’d do anything to prevent those errors, I’d much rather ensure that npm i && npm t works on a clean clone.

@mkhl
Copy link
Contributor

mkhl commented Apr 3, 2018

Oh, as a side note: I think I’d also symlink cli-harness.sh into the directories where it’s loaded in order to present a simpler environment to the test scripts.

@FND
Copy link
Contributor Author

FND commented May 10, 2018

Done: faucet-pipeline/faucet-pipeline-js@9c80dbb (with similar commits in faucet-core, faucet-sass, faucet-static, faucet-images and nite-owl)

@FND FND closed this as completed May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants