Shifter coverage file generation fix #118
Conversation
|
It seems there is a test failure, weird. I am looking into it now. |
|
Can anyone please have a look at this pull request? This is urgent to our development team. Thanks! |
| .log('coverage file read, starting coverage for: ' + fileName + '/' + fileName + '.js') | ||
| .coverage({ | ||
| type: coverageType, | ||
| charset: 'utf8', | ||
| name: 'build/' + fileName + '/' + fileName + '.js' | ||
| name: path.relative(process.cwd(), filePath) |
There was a problem hiding this comment.
I'm worry about this line. If what you're looking for is to be able to instrument files in multiple build folders, it might be ok, but it worries me that process.cwd() is not always relevant since you have the ability to pass a custom CWD from the command line, as well as the cascade execution of shifter spawn processes. I will look into more details see if I can get to the bottom of it.
There was a problem hiding this comment.
I agree about the process.cwd() brittleness. The hard-coding of 'build/' + definitely looks problematic, given the logic behind the initial queue.read() call. Wouldn't simply using the new filePath variable be sufficient here?
There was a problem hiding this comment.
yeah, since this is the report path, filePath should be good, it just happen that it might be relative or absolute, if it is relative, and you're testing multiple build folders, istanbul might get confused since two relative paths can be equal. You can easily solve that by making them absolute paths before calling shifter, by passing --build-dir.
There was a problem hiding this comment.
If filePath is used here, and it's absolute path, it will make testing much harder. Currently tests compare build with buildExpected, which means coverage files in buildExpected dir should reflect absolute path as well.
|
Any new thoughts, guys? I can surely change to use |
|
|
? I changed to use shifter.cwd() here, just like how it's used in other places in the same file.
|
Can anyone please come back to this PR. It's been 20days. |
|
@leik sorry dude, I'm on the road, hard to catch up with all PRs, will get back to this next week. In principle it looks ok, but I will have to dig in more. |
|
@leik I found an issue after merging this. when using shifter with yogi, |
|
ISTR coming across this issue in #103. The fix that I tried to implement there was to use path.relative to calculate the path between shifter.cwd() and the buildDir. |
|
Yeah, I suspect there is not an issue solution on this. |
|
Hi @caridy, even though this PR is closed, https://github.com/yui/shifter/blob/master/lib/module.js#L397 is definitely a severe bug in shifter, which has stopped us using shifter to generate code coverage files. I don't think shifter has to force developers to use "build" as their build output folder. Like other places in the same file, they use shifter.cwd(), I don't see why we cannot use it here in buildCoverage function. Don't you think it might be something wrong with Yogi if that's the only concern. Shifter is an individual build tool that people unnecessarily use with Yogi that stops us fixing this showstopper bug. We really need this fixed please, Thanks a lot. |
|
@leik I understand, but so far, no one has come up with a proper solution to calculate the proper relative path or an argument that describes the path. If a PR comes, and it works with the current yui build, and other builds, I will give it a try for sure. That being said, I think shifter is trying to do too many things (of course, keep in mind that it is a tool build before grunt, gulp and all other tools out there). So, you should simply write a grunt task to execute shifter then execute a coverage process for all files using the proper path for each file. Does that makes sense? |
|
Agreed @caridy - it must be a relative path rather than absolute, but IMO it needs to be relative to the project root. The patch that I put together in #103 does calculate a relative path which is relative to the project root (if it can be determined and has been requested), and uses the buildDir too, though I realise that it's not a perfect solution for all. I understand your opinion with regards making Shifter do fewer tasks, but there's also a major benefit to having fewer tools involved. For each tool which is added to the toolchain as a build requirement, it becomes progressively harder for new developers to a project to get on board and up to speed. For us, that's a major factor, and I imagine the same can be said for many open source projects using YUI. Equally, I think the same can be said for any project whose primary language is not JavaScript. If your main toolset is in php, ruby, python, etc. there's already plenty to do and there's a strong chance that some of the developers writing JavaScript won't be doing so as a primary language. Correct me if I'm wrong, but if you were to remove the coverage support from Shifter, and force us to write our own grunt task, we would effectively end up re-writing Shifter: The coverage files need to have a source map relating to the source files, not the build files, but they also need to be a working build of the module, complete with all of the standard boilerplate added by Shifter already. Effectively, you'd have to write a task which duplicates Shifter in every way except it would spit out -coverage.js (instead of, or as well as) the other files. Does that make sense? |
|
No, you're wrong. As today, coverage has nothing to do with the source code. Effectible it will be the same to write a grunt task that everyone can use that looks for https://github.com/yui/shifter/blob/master/lib/module.js#L390-L408 |
|
In which case, my apologies. For some reason I had it in my head that it used the src files (which would be handy in some ways). |
|
alright, now that we are all in the same page, we just need a volunteer to implement the grunt task, or try out the existing one: https://github.com/daniellmb/grunt-istanbul-coverage |
|
note: this PR was rolled back because of the details provided above. |
When generating coverage files, shifter uses hardcoded path
'build/' + fileName + '/' + fileName + '.js'instead of using mod.buildDir here. This is to fix this issue. Otherwise, when build output folder is not "build", Istanbul HTML report generation through grover fails and complains about no file found under that invalid path.