search

jejacks0n / teaspoon

Teaspoon: Javascript test runner for Rails. Use Selenium, BrowserStack, or PhantomJS.
1.43k stars 243 forks source link

[WIP] Ignored instrumentation files #322

Closed davestevens closed 9 years ago

davestevens commented 9 years ago

This is an initial attempt at including a configuration option to filter files being instrumented by Istanbul.

I've added the configuration at the top level as the coverage configuration is based on a suite which it not available from within Teaspoon::Instrumentation. This is currently allowing me to define a list of strings which are then used as regular expressions to check against the assets pathname.

Initial discussion was here: #315.

jejacks0n commented 9 years ago

Looks like a good start.. can we put this in the coverage specific configuration? Seems like it's pretty much directly tied to that.

jejacks0n commented 9 years ago

I appreciate the effort, and will merge it when it's moved into that area of the configuration. =)

davestevens commented 9 years ago

I was unsure about where it would go in the coverage specific configurations. If it goes within the Coverage class then its linked to a suite, which is unknown from within Instrumentation. Where do you think it should go? If it put it in Coverage and then look up the configuration assuming default as the suite name?

jejacks0n commented 9 years ago

haha, fair enough. I hadn't thought about that. pfff... thinking on that, but thanks for making the point.

jejacks0n commented 9 years ago

@mikepack how do you feel about this conversation? Do you think it's really bad if we set the current_suite at a Teaspoon level from within the controller so the instrumentation can determine what files to instrument or not? We're hobbled by how sprockets hooks work.

mikepack commented 9 years ago

@jejacks0n I'm very unfamiliar with this part of the codebase. We should stay away from a current_suite, IMO. What prevents us from calling Teaspoon.coverage_configs? Is it that we need to know what coverage config is being used (eg default or CI)?

jejacks0n commented 9 years ago

Because you can specify a coverage config at runtime, or by configuring one within your suite. It gets complicated. @davestevens how about I leave this open for a bit, and when we're working on it for the v1.0 release, we'll try and figure out how to do this nicely, so it just works. Are you cool with that?

jejacks0n commented 9 years ago

I'm thinking setting the current_suite is really the only way to accomplish it.. so, I dislike requirejs, have I said that already? haha. ;-P

davestevens commented 9 years ago

@jejacks0n thats fine with me. I'll make changes based on the way the matcher works and the other feedback.

jejacks0n commented 9 years ago

Ok, so I appreciate your effort on this, and sorry I haven't merged it yet.

There seems to be a solution that'll work better than this, and you have my apology for leading you down a less optimal path. I'm happy to do this work myself now, but I'll explain it anyway in case you want to give it a shot -- it's not far from this implementation really.

Ok, so instead of putting it at a top level configuration, it should be a coverage configuration option.. essentially just moving no_config from suite to coverage. The reason for this, is that the coverage configuration is known globally, and so after some thought it seems like it should technically belong within the coverage configuration anyway. This change does involve more changes than what's in this PR to make the suite understand these concepts through the coverage configuration.

davestevens commented 9 years ago

Thats fine, and it does make sense to have this option on the coverage configuration.

This is where I had originally put the option, but I ran into the issue of being unaware of which suite was currently being run when Teaspoon:: Instrumentation.add? was executed. What is your idea for this?

jejacks0n commented 9 years ago

It's not associated to the suite. It's set at the global use_coverage configuration level. That configuration can be specified at run time as well.

Jeremy Jackson

On Mar 5, 2015, at 2:22 AM, Dave Stevens notifications@github.com wrote:

Thats fine, and it does make sense to have this option on the coverage configuration.

This is where I had originally put the option, but I ran into the issue of being unaware of which suite was currently being run when Teaspoon:: Instrumentation.add? was executed. What is your idea for this?

— Reply to this email directly or view it on GitHub.

mikepack commented 9 years ago

@davestevens thanks to this PR, we've had some discussions internally and have plans to rework the coverage configuration for our 1.0 release (#308). We're thinking of removing all coverage options at the suite level, ie. moving the no_coverage to the coverage block.

Going to close this PR. Happy to reopen if you want to take a stab at getting this into the coverage block. Otherwise, we'll address it in the not-so-distant future.