search

nucleus-js / design

This repo is for the core design, discussion, spec, and tests for nucleus implementations.
Other
110 stars 20 forks source link

Move assert function to common.js file in test dir #15

Closed theworkflow closed 8 years ago

theworkflow commented 8 years ago

Pass message to assert functions in test-nucleus-get-set-env.js Move assert function to common.js file in test dir

creationix commented 8 years ago

This shouldn't work. The relative path just happens to work because I'm not protecting escaping from the bundle directory. Can you move the common folder to inside the parallel folder? Then use full paths to the file when using dofile. dofile("common/assert.js")

When we use the custom main feature, the bundle root becomes the directory holding the custom main.

Fishrock123 commented 8 years ago

@creationix It does work though. My other tests already use it - to access the fixtures directory from parallel. (They do not necessarily depend on it, but not having this features makes these things very messy.)

Is there a reason we must use a bundle directory? It seems that is only useful if we are running in zipped mode?

Fishrock123 commented 8 years ago

@creationix Should we open a new issue to discuss requiring bundled mode when not using zip files in more detail?

creationix commented 8 years ago

@Fishrock123 that is a good question. I suppose that custom main is already different behavior than running in zip mode. I just don't want people getting the false idea that reading outside the bundle will continue to work when they zip their app.

Fishrock123 commented 8 years ago

@ashleygwilliams Think the above may be solvable by clear, visible documentation? Or should we take some other action?

@creationix Could it be possible to make specialized messages for read-out-of-bundle when in "zipped mode"?

Fishrock123 commented 8 years ago

@harlanj looks good on the code side minus some minor things!

Fishrock123 commented 8 years ago

Might hold this a day to settle https://github.com/creationix/nucleus/issues/7 (license silliness for future-proofing)

creationix commented 8 years ago

I just don't want people getting the false idea that reading outside the bundle will continue to work when they zip their app.

Funny story. Today at work we had an incident with the luvit agent. There was a bug where a path had one too many dots ('..' vs '.') and was reading a file outside the bundle root. It passed all the tests since the tests ran in luvi mode, but when the agent was deployed, it crashed.

So my fear of differing behavior causing trouble wasn't unfounded after all.

I'm fine, however, with adding an explicit flag or something that allows overriding the root path. Or maybe we can have it look for a main.js in parent directories and automatically finding the app root.

Fishrock123 commented 8 years ago

I've been pretty busy. Will open a new issue for this discussion tomorrow.

Fishrock123 commented 8 years ago

Looks good, just pending other discussions.

theworkflow commented 8 years ago

Just wanted to check and see if there was any update on this @creationix @Fishrock123

Fishrock123 commented 8 years ago

Sorry, I think it should be merge-able. In the future we'll just run the tests with something like the rustyduk implementation's --no-bundle.

I'll try to take a full look when I can!

(p.s. There are lots of good issues that could be picked up in rustyduk if you are interested in JS & Rust: https://github.com/nucleus-js/rustyduk/issues)

creationix commented 8 years ago

I'm fine merging this now if @Fishrock123 is ok with it.

Fishrock123 commented 8 years ago

yeah looks good to me

I still need to get rustyduk on these days but that is for a different thread..

theworkflow commented 8 years ago

Cool. Thanks guys!