Allow giving detox config file as param, with fallbacks #906
Conversation
|
Anyone help me out on why this is failing? |
|
Android tests always fail that I can see. This PR is about fixing that #910 so yeah unrelated. |
detox/local-cli/detox-build.js
Outdated
|
|
||
| function getConfigurationFile(configPath) { | ||
| let config; | ||
| if (configPath) config = require(path.join(process.cwd(), configPath)); |
There was a problem hiding this comment.
What would happen if configPath is not a relative path, say like /Users/me/myConfig.json?
There was a problem hiding this comment.
right, added if/else on isAbsolute()
detox/local-cli/detox-test.js
Outdated
| } | ||
| } | ||
|
|
||
| function getConfigurationFile(configPath) { |
There was a problem hiding this comment.
Why the need for duplication? Can't you use the same function?
There was a problem hiding this comment.
right, added a util method
|
Thanks for this PR! |
|
@oreporan, firstly, thanks for submitting the PR. 👍 I'd also love to see this feature in Detox. However, from my perspective, it still lacks a few things to get it merged, including but not limited to the updates to Detox CLI documentation. I'll review it more thoroughly within the next few days, and get back to you with a list of TODOs. Cheers! |
|
Let me know what to do, I'd like to get this in, in the near future |
|
Anyone had a chance to look at this? PR has been open for over a month now |
|
Hey @oreporan, I have a few nitpicks (naturally) :)
Thanks! |
|
@rotemmiz However I did add it to now to |
docs/APIRef.DetoxCLI.md
Outdated
| @@ -72,7 +72,7 @@ Run a command defined in 'configuration.build' | |||
| | --- | --- | | |||
| | -h, --help | output usage information | | |||
| | -c, --configuration \<device config\> | Select a device configuration from your defined configurations,if not supplied, and there's only one configuration, detox will default to it | | |||
| | --config-path \<configPath\> | Select a device config-file path, if not supplied, detox will default to the package.json, and if not found there, detox will fallback to .detoxrc.json | | |||
detox/local-cli/detox-test.js
Outdated
| @@ -21,6 +21,8 @@ program | |||
| 'Disable colors in log output') | |||
| .option('-c, --configuration [device configuration]', | |||
| 'Select a device configuration from your defined configurations, if not supplied, and there\'s only one configuration, detox will default to it', getDefaultConfiguration()) | |||
| .option('--config-path [configPath]', | |||
| 'Select a device config-file path, if not supplied, detox will default to the package.json, and if not found there, detox will fallback to .detoxrc', getDefaultConfiguration()) | |||
There was a problem hiding this comment.
@oreporan , why is getDefaultConfiguration() is called for both parameters? Shouldn't it be a .detoxrc or something like that?
| if (!config) config = require(path.join(process.cwd(), packageJson)).detox; | ||
| if (!config) { | ||
| try { | ||
| config = require(path.join(process.cwd(), detoxRc)); |
There was a problem hiding this comment.
Why try is used only here? Did you mean fs.existsSync() ?
| function getConfigurationFile(configPath) { | ||
| let config; | ||
| if (configPath) config = require(path.isAbsolute(configPath) ? configPath : path.join(process.cwd(), configPath)); | ||
| if (!config) config = require(path.join(process.cwd(), packageJson)).detox; |
There was a problem hiding this comment.
All this require(path.join(process.cwd(), blabla)) looks repetitive and can be refactored to a method.
There was a problem hiding this comment.
Also, I don't like these tricks with path.isAbsolute() ? ... : .... I think that this method is more appropriate: path.resolve([...paths]) - Path | Node.js v10.11.0 Documentation
| if (!config) { | ||
| try { | ||
| config = require(path.join(process.cwd(), detoxRc)); | ||
| } catch (error) {} |
There was a problem hiding this comment.
Why do you suppress the error? If users have a malformed .detoxrc, how do you let them know about that?
Can you test whether the exceptions thrown on malformed or missing jsons are actually readable and clear? Good if you attach screenshots of the detox cli exceptions on these cases.
| return config | ||
| } | ||
|
|
||
| module.exports = getConfigurationFile; |
There was a problem hiding this comment.
Not sure how I can test these changes other configurations. As it seems that there is 1 app which is being tested, and it has its config in the package.json
detox/local-cli/detox-test.js
Outdated
| @@ -204,8 +207,9 @@ function clearDeviceRegistryLockFile() { | |||
| } | |||
|
|
|||
| function getDefaultConfiguration() { | |||
| if (_.size(config.configurations) === 1) { | |||
| return _.keys(config.configurations)[0]; | |||
| const file = getConfigurationFile(); | |||
There was a problem hiding this comment.
I counted that getConfigurationFile() is actually called three times here. It does not look simple and concise, can it be rewritten in a cleaner way?
|
trying to re-run the tests locally fails in: I've recently updated to xcode 10 |
|
@oreporan , it's not Detox issue AFAI can see. Have you tried to google it? |
detox/local-cli/detox-build.js
Outdated
| .option('--config-path [configPath]', | ||
| 'Select a device config-file path, if not supplied, detox will default to the package.json, and if not found there, detox will fallback to .detoxrc') | ||
| .option('-c, --configuration [device configuration]', 'Select a device configuration from your defined configurations,' + | ||
| 'if not supplied, and there\'s only one configuration, detox will default to it', getDefaultConfigurationFile()) |
There was a problem hiding this comment.
@oreporan , why getDefaultConfigurationFile()? Come on, it is a string like android.emu.release, ios.sim.debug, why require file? It is the second time I write it to you.
|
@oreporan , yes, something like that. Maybe, at the end of the day, detox does not need arguments in |
|
@noomorph Maybe this is a separate PR? |
detox/local-cli/detox-test.js
Outdated
| @@ -204,8 +207,9 @@ function clearDeviceRegistryLockFile() { | |||
| } | |||
|
|
|||
| function getDefaultConfiguration() { | |||
| if (_.size(config.configurations) === 1) { | |||
| return _.keys(config.configurations)[0]; | |||
| const file = new ConfigurationResolver().getDetoxConfiguration(); | |||
There was a problem hiding this comment.
Why instantiate ConfigurationResolver many times? I see there is already ConfigurationResolver.default in the code, so you can require it.
| @@ -0,0 +1,57 @@ | |||
| const fs = require('fs'); | |||
There was a problem hiding this comment.
You know... since the number of possible run environments is limited to 4, it would make sense to create four folders in __mocks__/configuration-resolver-it/, e.g.:
__mocks__/configuration-resolver-it/detoxrc-and-package/__mocks__/configuration-resolver-it/detoxrc/__mocks__/configuration-resolver-it/package/__mocks__/configuration-resolver-it/blank/
I think tests would be more readable then, with less hassle about creating the environment dynamically.
Also, there are no tests for unhappy paths when a resolved file is not found or is malformed.
| } | ||
|
|
||
| loadDetoxrcConfiguration() { | ||
| var data = fs.readFileSync(this.resolvePath(detoxRc)).toString(); |
There was a problem hiding this comment.
Is there a specific consideration of why you want to limit .detoxrc to JSON format only?
Personally, I'd love to be able to evaluate it as JS, I'm confident that some people would be happy too and sure they'll find their way how to leverage that.
There was a problem hiding this comment.
So what do you suggest ? this can (and should) be separated and added in a later PR IMO.
There was a problem hiding this comment.
this == js evaluated config (.detoxrc.js, detox.config.js, etc)
There was a problem hiding this comment.
@rotemmiz , I suggest using plain require() instead of JSON.parse(fs.readFileSync...
|
@oreporan , I think if you make this place less stricter... https://github.com/wix/Detox/blob/master/detox/src/index.js#L41 ...then it won't quite be a breaking change, rather a new feature. You can use |
|
@oreporan, because, otherwise... what is the sense in this pull request - without config resolution on the test runner side? I don't see scenarios where it can be helpful while being half-baked... if I am correct, then it would just always get overridden by what's in As for the situation where the |
|
Hey! Can we wake this PR up? I want to see it passes CI so we can accept it! There's so much work here, would be shame if it goes to waste. |
|
@rotemmiz, I like this PR as well, but it's not finished. There's still detox.init() rewrite pending. I'm wondering if I might continue the work to complete it. 😕 |
|
sorry, been busy - didn't have much time to work on it |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Commenting to keep the PR alive... Improving detox's configurability would be very useful. Having to hard code things in package.json is often inconvenient. |
|
The issue has been closed for inactivity. |
|
Reopening. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
The issue has been closed for inactivity. |
|
This seems dead and there is another PR (#1238) on this subject. Closing |
Solves issue #901
I couldn't really add specific tests for this, maybe you can help me out on where.
Because the e2e is basically one project, so its already taking the config from the
tests/package.jsonand I'm not sure adding another test just for this is justified