Add instrumentation for RequireJS

[davestevens]

Uses urlArgs to include instrument=1 to urls when coverage is required. See: http://requirejs.org/docs/api.html#config-urlArgs

Added waitSeconds: 0 as on my Project I was getting timeouts when it was attempting to add instrumentation to all my files.

NB: I created this in a local version of Teaspoon which was using version 0.8.0, the output showed all the files and I was able to use the html report to view the files. I have just tried this on master and it does not seem to load any files. Is there something thats changed between 0.8.0 and master that requires different config?

[jejacks0n]

Yay! this is totally better, IMHO. I'll merge, and you can confirm that your setup is working on master?

Thanks for this.

[davestevens]

Your last comment on #277 made sense, RequireJS shouldn't need modification deep down in your code. I'm just running though git bisect now to figure out where the coverage seems to be broken.

[davestevens]

507ef1226be42fd1df140bf92f4dda1a6bef11e4 is the first bad commit
commit 507ef1226be42fd1df140bf92f4dda1a6bef11e4
Author: jejacks0n <jejacks0n@gmail.com>
Date:   Wed Aug 20 15:00:57 2014 -0600

    forces update of instanbul -- might cause problems, may need to do a version check.

:040000 040000 28c319f39ba616ae9516e5360028f67b790b4ae4 ba3bfdf81bb467ee6dba976b200014716c50f197 M      lib
:040000 040000 d6c58a40cd347005edcd96ec073b45df40dce0e2 7e5d4391464ecf3c3131edf4ef30f1c06bf46bd2 M      spec

Are the docs right regarding coverage? I've got

Teaspoon.configure do |config|
  ...
  config.coverage do |coverage|
    coverage.reports = ['text', 'html', 'cobertura']
  end
  ...
end

in the Teaspoon env file.

[jejacks0n]

that should be correct to generate reports.. there was a difference of instanbul arguments (if I recall correctly) and I could not find a way to resolve the versions. You should be running the latest istanbul for the command to work correctly.

[jejacks0n]

I never added a version check because there had been a pretty long lived deprecation warning from istanbul directly.. and also, it was hard to check version (I think this too had changed with the versions), and so would've been ugly and painful.

[davestevens]

Okay, I was on 0.1.44, just upgraded to 0.3.6. I hacky way of getting the version is:

$(which istanbul) help 2>&1 | grep "version" | cut -d \: -f 2

[jejacks0n]

I'm guessing that doesn't work on windows. heh. ;-P

We should probably indicate the required istanbul version in the readme though, so point taken. If it's working correctly, do you want to provide a follow up PR that specifies the required istanbul version?

Thanks again for your help in resolving this -- I want to support requirejs, but don't use it myself, and that makes supporting a thing much harder than otherwise.

[jejacks0n]

Also, want to make sure the wiki is up to date.. Would you do a quick review and add anything that might be wrong/left out?

https://github.com/modeset/teaspoon/wiki/RequireJS-with-Teaspoon

[davestevens]

Awesome, that works now. It includes all of the spec files too, but all is better than nothing I guess.

I'll have a look through the docs and update anything that needs doing.

No worries about the help, to be honest I can't believe I didn't think of this when I was looking before. It doesn't seem to matter if you use it or not, all of us who do didn't know how to get it working either!

PS: good point about windows!

[jejacks0n]

let's discuss adding a global ignored_files to the coverage configuration. I think this will allow suite level overrides, but will provide a basic set that will resolve this specific problem.

[davestevens]

Thats probably the nicest way around it. As an example SimpleCov has an add_filter method that will not include any files based on the string defined: https://github.com/colszowka/simplecov#defining-custom-filters

[jejacks0n]

The other way we may experiment with, is setting the current suite into the configuration -- though this gives me pause. If the current suite being rendered is accessible globally, then we can use that configuration directly in the instrumentation layer. This is the less optimal approach, but might provide a simpler mechanism to achieve the desired result in the current architecture.

[davestevens]

I was just looking into adding an ignored_files option for the coverage config and realised that there is already a method of doing this. suite.config.no_coverage << "/spec" works for what I want.

[jejacks0n]

yeah, there is that, but the question has to be asked at the time the files are being linked, as the suite is not known at the time that assets are requested.. does that make sense? usually, when we build out the manifest of files to load, we have the suite config in hand, but at the time the assets are loaded, it's expecting the instrument param to be present or not -- and there's not an easy way to ask that question at the time that the asset is being loaded -- nor does that fit the current flow cleanly.

[davestevens]

I did notice that I didn't have access to the suite in the instrumentation file. Does this mean that all files will be instrumented but then the reports are filtered out as apposed to not instrumenting the files that are not not required, which should be quicker?

I'll have another look at this over the week. What would you suggest? Putting an ignored_files option on the @@coverage_configs variable? That way its global and doesn't depend on any suite.