Switching instrument_file? to be in Instrumentation class

[golmansax]

This was done to solve the RequireJS issue (https://github.com/modeset/teaspoon/pull/240), would like to hear your thoughts about this. Some things:

• The reason why this change works with RequireJS is because the decision to instrument is now made in the Sprockets hook, and all RequireJS requests go through Sprockets.
• I wasn't sure how to grab the current config so I grabbed the default config with Teaspoon.configuration.suite_configs['default'][:instance]. I can help write specs once this part is figured out.
• I was thinking about completely forgoing the addition of instrument=1, but wasn't sure which way to go. Either way works with RequireJS, as you can add a instrument=1 to the end of every RequireJS request through the urlArgs config.

[coveralls]

Coverage decreased (-0.59%) when pulling 50a35cb99d981d2a3a52b89b4256394b3f02999f on golmansax:master into 32d3385042f2c5295e8f74955559e29c155b93e5 on modeset:master.

[jejacks0n]

I'll give this a try and revert if needed.

[jejacks0n]

I removed the stuff you had shifted out of suite, because it was only using the default configuration -- which broke some specs, and seems short sighted.

[jejacks0n]

I now understand what the changes were, but don't fully understand the need for them.. can you explain further?

[jejacks0n]

If I understand correctly, you need to instrument files that are being loaded via requirejs, and thus the instrument=1 param isn't present. I'll poke around a bit, and understand the desire for it, but absolutely abhor having to do shims in teaspoon to support requirejs -- it's at an entirely different level, and one I happen to disagree with.

If it can come at a different layer, it would be better. So the ideas I've kicked around, but can't quite resolve are to add an additional configuration -- but even that doesn't work well, and the core reason is that the instrumentation determination has to come at the level when a file is included for load -- with all the context of the configuration used during the test run -- which is not possible when an asset is requested.. though I'll investigate that.

[davestevens]

I've just opened #315 that simply adds an option to RequireJS from within the boot view which adds instrument=1 to all requests made through require.

[golmansax]

I'm sorry I probably can't explain the reasoning as well as I could have, since I haven't looked at this code for a while now haha. I think your analysis is pretty on point, here is my best guess to what I was thinking at the time:

• I forget exactly why, but I believe the requirejs files didn't get the &instrument=1 through Teaspoon::Suite#asset_url. However, you can around it in requirejs by adding instrument=1 to all files through the config like what @davestevens said
• So then, all of the files have instrument=1, but the no_coverage files were not filtered out. I guess this is why I switched the no_coverage check to Teaspoon::Instrumentation (which can filter the requirejs files because requirejs also goes through Sprockets)

Sorry for the code sloppiness - this was meant to be more of exploratory pull than one with production ready code. Just wanted to get your thoughts on it because I wasn't really considering how this would impact the non-requirejs use cases of teaspoon.