Skip to content

Conversation

@ryoqun
Copy link

@ryoqun ryoqun commented Oct 27, 2016

TBD

@ryoqun ryoqun changed the title Expose needed API for coffeescript's self-coverage [wip] Expose needed API for coffeescript's self-coverage Oct 27, 2016
@ryoqun
Copy link
Author

ryoqun commented Nov 4, 2016

@jwalton FYI, I'm currently working for coffee-coverage to be officially supported by coffeescript: jashkenas/coffeescript#4348

instrumentor: 'istanbul',
basePath: process.cwd(),
exclude: ['/test', '/node_modules', '/.git'],
exclude: ['/node_modules', '/.git', '/documentation', '/examples'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is '/test/' not excluded anymore?

Copy link
Author

Choose a reason for hiding this comment

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

Well, this is my personal preference.. It's that the code coverage of test files should be available too.

That's because there have been countless times when the new-written tests are not correctly run in my development.. Under aggressive development things progress at a rapid pace, so it's be difficult to be careful always to every detail.

Also, because coverage information can be provided as a no additional manual effort, this can be useful for time-effective just hurried-up investigation to fix something wrong as much quick as possible.

Under that situation, just seeing if coverages are intentionally collected is just a plus, I think. :) Also, as a developer, while doing TDD, it's always fun to see my laboured code painted as green regardless of application or test code.

Anyway, that's my two cents. I'm persuading no one. Before the wip status got removed, I'll revert this change. :)

replaceHandler = (extension) ->
origCoffeeHandler = require.extensions[extension]
# just expose needed API in some hideous way....
fs.instrumentFile ||= (fileName) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding this to fs? O_o Seems like adding this to the main coffeeCoverage export would be a better plan?

Copy link
Author

Choose a reason for hiding this comment

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

@jwalton you're absolutely correct. :) At that time, I was just trying the stuff to work.. So, I abused the fs... :p

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants