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

Additive configuration is broken in some instances. #877

Closed
jason0x43 opened this issue Feb 7, 2018 · 4 comments
Closed

Additive configuration is broken in some instances. #877

jason0x43 opened this issue Feb 7, 2018 · 4 comments
Labels
bug Something that's not working as intended effort-medium This may take a couple of days priority-medium This should get done, but it's not a high priority

Comments

@jason0x43
Copy link
Member

In Intern 4.1.5, having a config with "reporters+": "htmlcoverage" will result in only htmlcoverage being active in the Node executor, when the default runner reporter should also be active. The issue is that config data is being resolved by getConfig rather than the executor's own config method, so the executor ends up missing some useful information (like the presence of the '+' indicator).

@jason0x43 jason0x43 added the bug Something that's not working as intended label Feb 7, 2018
@jason0x43
Copy link
Member Author

The expected result is that htmlcoverage is added to the default reporter (runner).

@aciccarello
Copy link
Contributor

So it would be complicated to rework the config loading to return the addToExisting property since it is dependent on config order. Even if the addToExisting was retained, you would need to also pass each level of config and reimplement to logic to combine them. This is because the main config may be additive but then the child config could completely override it.

Config load order (simplified)

  1. Defaults (needed)
  2. Extended
  3. Main
  4. Child (node/browser)
  5. Args

In order to support defaults like the reporter, the config loader should take a default config argument. This would probably need to be a breaking change since the config loading functions already have multiple optional params and the loadConfig() function is publicly exposed.

Relevant files in loading config:

  • bin/intern.ts which requests the config and sets it on intern
  • lib/node/util.ts getConfig()
  • lib/common/util.ts loadConfig() and the recursive implementation function _loadConfig()

@jason0x43
Copy link
Member Author

Would the default config then take the place of the defaults being applied by the executors? That would simplify things, particularly around the handling of default reporters.

As far as breaking changes go, we can tolerate them for Intern 5-pre (yay major version change). It may be worth considering how we could minimize those in the future, though, possibly through more use of options objects vs additional discrete arguments.

@jason0x43 jason0x43 added effort-medium This may take a couple of days priority-medium This should get done, but it's not a high priority labels Jan 29, 2020
@jason0x43
Copy link
Member Author

Closing this in favor of #1109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that's not working as intended effort-medium This may take a couple of days priority-medium This should get done, but it's not a high priority
Projects
None yet
Development

No branches or pull requests

2 participants