jejacks0n / teaspoon

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

Remove upper bound on railties #557

Closed KaanOzkan closed 5 years ago

KaanOzkan commented 5 years ago

Railties version needs to be bumped to 6.0.0.beta1 to use teaspoon with Rails 6.

I removed the upper bound constraint altogether to be less conservative about this. If you want to be safe we could also change it to a < 6.0 constraint.

KaanOzkan commented 5 years ago

@jejacks0n Failure seems to be the previous flaky test. Let me know which way you prefer.

jejacks0n commented 5 years ago

Maybe it's not a flaky test?

I see this: "You requested coverage reports, but no results were found. Are all files being ignored in your coverage config? If you have expand_assets set to false, you will need to remove spec_helper from the ignore list."

KaanOzkan commented 5 years ago

This error is from the same test but it is a different failure than last one as you noticed. I wasn't able to reproduce locally, so I removed my changed to trigger CI. It is still the same error. I am not sure where it is introduced. It is raising a CoverageResultNotFoundError https://github.com/jejacks0n/teaspoon/blob/8be60fe2a9445cec58687ae3fb0b73f60fb0c691/lib/teaspoon/coverage.rb#L19

(https://travis-ci.org/jejacks0n/teaspoon/builds/496170531)

KaanOzkan commented 5 years ago

@jejacks0n I have been running some experiments in order to reproduce the issue. CoverageResultNotFoundError is raised from the Coverage initializer. And it seems to be only triggered from 2 places. First in the Runner and second in Configuration. Locally I am not even able to hit these code paths upon a console_spec test, let alone reproduce. I wanted to ask for your help since I am not sure what run_teaspoon() touches in inside our failing console_spec. \ \ TL;DR Code that could lead to an error isn't being triggered in local test. I don't see any reason why CoverageResultNotFoundError is thrown on the CI build.

jejacks0n commented 5 years ago

So, it’s using istanbul to generate that stuff. I assume locally you might not have istanbul installed, in which case maybe I or someone else disabled that spec if it’s not available to be tested? Probably a no-win situation with that spec — requiring everyone to install istanbul, or expect ci to catch things, but in either case, that’s the likely the crux of it.

https://istanbul.js.org/ https://istanbul.js.org/

You can install it, and if it’s detected to be present in your path (and your suit is configured to use it), it’ll run specs and code through istanbul to be instrumented to gather coverage details.

There is a monkey patch into sprockets that might have been broken with rails 6.

https://github.com/jejacks0n/teaspoon/blob/master/lib/teaspoon/instrumentation.rb https://github.com/jejacks0n/teaspoon/blob/master/lib/teaspoon/instrumentation.rb https://github.com/jejacks0n/teaspoon/blob/master/lib/teaspoon/engine.rb#L53

On Feb 21, 2019, at 4:03 PM, Kaan notifications@github.com wrote:

@jejacks0n https://github.com/jejacks0n I have been running some experiments in order to reproduce the issue. CoverageResultNotFoundError is raised from the Coverage initializer. And it seems to be only triggered from 2 places. First in the Runner and second in Configuration. Locally I am not even able to hit these code paths upon a console_spec test, let alone reproduce. I wanted to ask for your help since I am not sure what run_teaspoon() touches in inside our failing console_spec.

TL;DR Code that could lead to an error isn't being triggered in local test. I don't see any reason why CoverageResultNotFoundError is thrown on the CI build.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jejacks0n/teaspoon/pull/557#issuecomment-466204311, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA1xUf0KYMaox4wCOZwfed1LE3H3d_4ks5vPyXVgaJpZM4bDa1D.

KaanOzkan commented 5 years ago

Hmm I was able to get the expected output locally so I'd assume Istanbul usage was working properly. Locally only part I have different than CI is manually calling bundle install --local on the testapp due to GemNotFound errors. I don't think that is related to being able to reproduce though.

I will definitely checkout that monkey patch, thanks. Same failure have popped up on both Rails-5 and 6 but it's gotta be somehow related to this upgrade. I'll keep digging.

KaanOzkan commented 5 years ago

@jejacks0n I was able to reproduce the issue locally and identify where the problem is occurring. Locally 50% of the time running phantomjs is returning a "line" hash without :coverage and thus causing an exception.

https://github.com/jejacks0n/teaspoon/blob/master/lib/teaspoon/driver/phantomjs.rb#L35

Same executable and args are passed in both runs. I am lacking context in 2 areas that problem might occur. Firstly it might be related specifically to internals of phantomjs executable. However, this wouldn't explain how we are able to get correct outputs half the time. Or related to passing this coverage option correctly to rest of the run. https://github.com/jejacks0n/teaspoon/blob/master/teaspoon-mocha/spec/console_spec.rb#L70 . Although phantomjs run doesn't seem to know about the coverage option passed in.

I would appreciate any kind of input!

KaanOzkan commented 5 years ago

Hey @jejacks0n, issue seems to be flakey and related to phantomjs and its return value. This comment has a bit more context: https://github.com/jejacks0n/teaspoon/pull/557#issuecomment-467936375

In CI builds error occurred for rails_5.gemfile and rails_4.gemfile's as well so I don't think it's specific to the change in this PR.