(feat) added argument for karma configuration file #4564
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
|
Heya @ruffiem, thanks for adding this. Can you add a test to confirm it works? You should add it here: The easiest way is to move the original config file and use your flag, like here: |
| } | ||
|
|
||
|
|
||
| const TestCommand = EmberTestCommand.extend({ | ||
| availableOptions: [ | ||
| { name: 'watch', type: Boolean, default: true, aliases: ['w'] }, | ||
| { name: 'code-coverage', type: Boolean, default: false, aliases: ['cc'] }, | ||
| { name: 'config-file', type: Boolean, default: false, aliases: ['c', 'cf'] }, |
There was a problem hiding this comment.
This should probably be config with an alias of c to be consistent with the upcoming e2e changes. (#4527)
There was a problem hiding this comment.
True but karma prefers --config-file... Which one then ?
There was a problem hiding this comment.
Personally, I'd prefer the CLI was consistent with itself. Especially considering karma may not always be the test runner.
Also, the type should be 'Path'.
|
Testing went ok! |
|
last commit related to #4577 |
tests/e2e/tests/test/test.ts
Outdated
|
|
||
| export default function () { | ||
| // make sure both --watch=false and --single-run work | ||
| return ng('test', '--single-run') | ||
| .then(() => ng('test', '--watch=false')); | ||
| .then(() => ng('test', '--watch=false')) | ||
| .then(() => copyFile('./karma.conf.js', './karma.conf.bis.js')) |
There was a problem hiding this comment.
Use moveFile instead of copy please, otherwise the feature might be broken and we don't know since the old file is used.
The test harness will take care of moving it back after the test.
| @@ -13,7 +13,7 @@ module.exports = Command.extend({ | |||
|
|
|||
| availableOptions: [ | |||
| { name: 'environment', type: String, default: 'test', aliases: ['e'] }, | |||
| { name: 'config-file', type: String, aliases: ['c', 'cf']}, | |||
| { name: 'config', type: String, aliases: ['c', 'cf']}, | |||
There was a problem hiding this comment.
No need to change this, our command overrides this one.
| } | ||
|
|
||
|
|
||
| const TestCommand = EmberTestCommand.extend({ | ||
| availableOptions: [ | ||
| { name: 'watch', type: Boolean, default: true, aliases: ['w'] }, | ||
| { name: 'code-coverage', type: Boolean, default: false, aliases: ['cc'] }, | ||
| { name: 'config', type: String, aliases: ['c', 'cf'] }, |
There was a problem hiding this comment.
Use just the c alias instead. I understand the old command had both, but we don't use it so no user would know and this way we don't use up another possible alias.
There was a problem hiding this comment.
Updated with your comments. Though, 'cc' and 'c' for respectively code coverage and config is little bit scary, no ?
There was a problem hiding this comment.
I think it's ok enough. It's true that -c -cc is kinda weird, but I'd expect -c to be used much more than -cc, and less so them together. And when users use them together, they shouldn't have to change -c to -cf.
Looking at the PoV of making mistakes, since before both -c and -cf were allowed then you could just as easily mistakenly do -cc instead if you were used to -c.
|
Just waiting on CI to merge. |
|
Thanks for this! It closes one of the oldest Angular CLI issues. |
|
How do I specify a different karma.conf.js for CI? |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Shuts #183