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

feat(config): instrumenter override option #79

Merged
merged 1 commit into from
May 22, 2014

Conversation

gvarsanyi
Copy link
Contributor

(As a result of PR-77 conversations)

Enable fine-tuning which preprocessor should be used for which files.
Add README.md section
Add test, update existing tests to comply with new dependency requirement

```javascript
coverageReporter: {
instrumenter: {
'**/*.coffee': 'istanbul' // No Ibrik/coffee preprocessing for this repo
Copy link
Member

Choose a reason for hiding this comment

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

I would say:

Force the use of the Istanbul instrumenter to cover CoffeeScript files

@aymericbeaumet
Copy link
Member

Good work.

One last thing to note: in order to avoid the initialization of a useless instrumenter, I would defer its loading (i.e. Istanbul/Ibrik loading) after the for loop used to determined which instrumenter should be used, and so only load the appropriate one. It's a small optimization, but since you're at it.

@gvarsanyi
Copy link
Contributor Author

Thank you.
I have updated the code and the readme file as per your request (with the exception of the process.exit(1) one, see my concern described in the line note conversation above).

@aymericbeaumet
Copy link
Member

I'll answer here as this diff is now outdated (and so hidden).

I totally agree with you, the use of return process.exit(1); would have been incorrect as we don't have the right to exit the process at this place. Thanks for noticing it.

A callback 'done' is passed to the function where the error could occur. You should be able to do what you want by calling it with the desired exit code.

@gvarsanyi
Copy link
Contributor Author

After some research now I think I get the idea behind done(). Still find it weird that one can't pass an actual error object/msg there, but alas: it does not break the async principals.

Updated the code to do the error handling now. Also moved out the check from the per-file cycle. Other then the smaller performance benefit it is needed to avoid having multiple error messages for the same error.

Thanks for the help, appreciate it.

@aymericbeaumet
Copy link
Member

Did you tried to pass an unknown instrumenter? Was the error code correctly forwarded to the karma.server.start callback?

@gvarsanyi
Copy link
Contributor Author

yes, I have tried, it comes back with exit_code === 1 for sthg like this if a bugous instrumenter name is passed in (0 otherwise).

karma = require 'karma'
karma.server.start options, (exit_code) ->
  console.error('Exit code:', exit_code) if exit_code > 0

@aymericbeaumet
Copy link
Member

Ok nice, could you please add a test case for it?

@gvarsanyi
Copy link
Contributor Author

Added

@aymericbeaumet
Copy link
Member

Correct the commit message and I'll merge it.

Enable fine-tuning which preprocessor should be used for which files
Add README.md section
Add tests
Update existing tests to comply with new dependency requirement
@gvarsanyi
Copy link
Contributor Author

hm, somehow the first line got lost in squashing. Fixed.

@aymericbeaumet aymericbeaumet merged commit ee3e68e into karma-runner:master May 22, 2014
aymericbeaumet added a commit that referenced this pull request May 25, 2014
The `coverageReporter.instrumenter` object can be passed to the karma
configuration to override the automatic instrumenters inferring (since this
[PR]). However, when the `coverageReporter` key was omitted, a type error was
emitted.

[PR]: #79
aymericbeaumet added a commit that referenced this pull request May 25, 2014
The `coverageReporter.instrumenter` object can be passed to the karma
configuration to override the automatic instrumenters inferring (since this
[PR]). However, when the `coverageReporter` key was omitted, a type error was
emitted.

[PR]: #79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants