Skip to content
This repository was archived by the owner on Sep 21, 2022. It is now read-only.

Repaired require for default .gemini.js and .gemini.json#418

Closed
twolfson wants to merge 1 commit intogemini-testing:masterfrom
twolfson:dev/fix.gemini.js.require.sqwished
Closed

Repaired require for default .gemini.js and .gemini.json#418
twolfson wants to merge 1 commit intogemini-testing:masterfrom
twolfson:dev/fix.gemini.js.require.sqwished

Conversation

@twolfson
Copy link

@twolfson twolfson commented Mar 31, 2016

I'm experimenting with a script that requires a dynamically computed .gemini.js (depends on node module for path to executable). However, when I ran Gemini I saw that it was getting a MODULE_NOT_FOUND error for .gemini.js despite it working earlier for .gemini.yml.

After some digging, I found that we were essentially running require('.gemini.js') which tells Node.js to look for a package named .gemini.js instead of the local file (i.e. ./.gemini.js). I have resolved that in this PR by moving to an absolute path for .gemini.js/.gemini.json.

In this PR:

  • Updated getDefaultConfig to return absolute path for non-YAML files

Notes:

We could use fs.readFile and JSON.parse/vm.runInContext (since fs.readFile resolves from process.cwd() but this is much simpler.

@levonet levonet added the review label Mar 31, 2016
@twolfson
Copy link
Author

twolfson commented Mar 31, 2016

Gah, I forgot to point to stable

@twolfson twolfson closed this Mar 31, 2016
@levonet levonet removed the review label Mar 31, 2016
@twolfson twolfson reopened this Mar 31, 2016
@levonet levonet added the review label Mar 31, 2016
@twolfson
Copy link
Author

twolfson commented Mar 31, 2016

Oh, never mind. There's a stable documented in CONTRIBUTING.md but it doesn't exist =/

https://github.com/gemini-testing/gemini/blob/v3.0.2/CONTRIBUTING.md

@coveralls
Copy link

coveralls commented Mar 31, 2016

Coverage Status

Coverage decreased (-1.0%) to 75.572% when pulling 6a34899 on twolfson:dev/fix.gemini.js.require.sqwished into ed2fae0 on gemini-testing:master.

@coveralls
Copy link

coveralls commented Mar 31, 2016

Coverage Status

Coverage decreased (-1.0%) to 75.572% when pulling 6a34899 on twolfson:dev/fix.gemini.js.require.sqwished into ed2fae0 on gemini-testing:master.

j0tunn
j0tunn reviewed Apr 3, 2016
View reviewed changes
lib/config/index.js Outdated
throw new GeminiError('No config found');
}

if (!/\.yml$/.test(configFile)) {
Copy link
Contributor

@j0tunn j0tunn Apr 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do it for yml file too

Copy link
Author

@twolfson twolfson Apr 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I initially left yml out to prevent long paths in case there was an error later and it popped up in the stack trace.

@twolfson twolfson force-pushed the dev/fix.gemini.js.require.sqwished branch from 6a34899 to 8d9e3c9 Compare April 3, 2016 23:09
@twolfson
Copy link
Author

twolfson commented Apr 3, 2016

Alright, I have addressed the comments and updated the PR:

  • Added YML absolute path resolution
  • Added tests for resolving a default config

@coveralls
Copy link

coveralls commented Apr 3, 2016

Coverage Status

Coverage decreased (-0.6%) to 77.489% when pulling 8d9e3c9 on twolfson:dev/fix.gemini.js.require.sqwished into f28ea8e on gemini-testing:master.

@j0tunn
Copy link
Contributor

j0tunn commented Apr 5, 2016

@twolfson Thanks, but I was thinking about some unit-tests may be. And thats not a good idea to make getDefaultConfig public only for test purposes.
I'll take a look in a few days

@twolfson
Copy link
Author

twolfson commented Apr 5, 2016

Ah, alright. I think using unit tests will be too overwhelming for me.

With respect to the current status, it's possible to have the same result in functional without getDefaultConfig but we would have to set a distinct variable. For example:

// In `default-js/.gemini.js`
projectRoot: '/it/works/default-js'

// In our tests
process.chdir('default-js');
var config = new Config();
assert.deepPropertyVal(config, 'system.projectRoot', '/it/works/default-js');

However, I will leave the PR as is until your exploration with unit tests.

BigBadAlien pushed a commit to BigBadAlien/gemini that referenced this pull request Apr 12, 2016
Changed require configuration to path relative to current working dir…
9879fe3 
…ectory

For example working directory is `/home/foo/project_with_gemini/gemini/`.
We use js or json default path configuration or use `-c` option.
Run `./gemini` or `./gemini -c .very_special_configuration.json`.
And we get configuration relative to current working directory:
/home/foo/project_with_gemini/gemini/.gemini.conf.json
/home/foo/project_with_gemini/gemini/.gemini.conf.js
/home/foo/project_with_gemini/gemini/.very_special_configuration.json
not in directory anywhere in the node_modules.
Fixed gemini-testing#418.
@BigBadAlien BigBadAlien mentioned this pull request Apr 12, 2016
Open
BigBadAlien pushed a commit to BigBadAlien/gemini that referenced this pull request Apr 12, 2016
Changed require configuration to path relative to current working dir…
8e6bf55 
…ectory

For example working directory is `/home/foo/project_with_gemini/gemini/`.
We use js or json default path configuration or use `-c` option.
Run `./gemini` or `./gemini -c .very_special_configuration.json`.
And we get configuration relative to current working directory:
/home/foo/project_with_gemini/gemini/.gemini.conf.json
/home/foo/project_with_gemini/gemini/.gemini.conf.js
/home/foo/project_with_gemini/gemini/.very_special_configuration.json
not in directory anywhere in the node_modules.
Fixed gemini-testing#418.
BigBadAlien pushed a commit to BigBadAlien/gemini that referenced this pull request Apr 12, 2016
Changed require configuration to path relative to current working dir…
a4fd607 
…ectory

For example working directory is `/home/foo/project_with_gemini/gemini/`.
We use js or json default path configuration or use `-c` option.
Run `./gemini` or `./gemini -c .very_special_configuration.json`.
And we get configuration relative to current working directory:
/home/foo/project_with_gemini/gemini/.gemini.conf.json
/home/foo/project_with_gemini/gemini/.gemini.conf.js
/home/foo/project_with_gemini/gemini/.very_special_configuration.json
not in directory anywhere in the node_modules.
Fixed gemini-testing#418.
@j0tunn
Copy link
Contributor

j0tunn commented Apr 20, 2016

Closing in favor of #442
@twolfson Sorry for long delay

@j0tunn j0tunn closed this Apr 20, 2016
@levonet levonet removed the review label Apr 20, 2016
@twolfson
Copy link
Author

twolfson commented Apr 20, 2016

Glad to hear it's been patched. Fwiw, #442 doesn't seem to have any tests to prevent this regression from recurring (i.e. nothing verifying default path is absolute) =/

@j0tunn
Copy link
Contributor

j0tunn commented Apr 21, 2016

Fwiw, #442 doesn't seem to have any tests to prevent this regression from recurring (i.e. nothing verifying default path is absolute) =/

https://github.com/gemini-testing/gemini/pull/442/files#diff-7cd03a08e3e1393e232f4490c33a335aR22

@twolfson
Copy link
Author

twolfson commented Apr 21, 2016

That line is a mock setup in the test, it doesn't verify that the actual config-reader code uses path.resolve =/

https://github.com/gemini-testing/gemini/pull/442/files#diff-435eaee6ef8a8a771ac5e6ca51dac9d6R26

@j0tunn
Copy link
Contributor

j0tunn commented Apr 22, 2016

Those tests will fail without that line. Take a look at this commit: 88de4a9

@twolfson
Copy link
Author

twolfson commented Apr 22, 2016

After playing around with it myself, I realized what is going on:

  • fs.readdirSync and proxyquire are emulating the filesystem and only defining absolute paths
  • getDefaultConfig must return an absolute path, otherwise it won't match the defined absolute path

As a heads up, while this works for now it will cause a maintenance tax any time the implementation is changed (e.g. moving to JSON.parse for JSON files will require a restructuring of the tests).

I should also note that that PR doesn't test .gemini.yml loading =/

@twolfson
Copy link
Author

twolfson commented Apr 22, 2016

And there's also no test for what happens when no config is found =/

@j0tunn
Copy link
Contributor

j0tunn commented Apr 25, 2016

Yep, we doesn't have 100% coverage. And we write tests when we see bug or on some code modification.
You can send PR if you wish.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@twolfson @coveralls @j0tunn @levonet