Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opal-rspec 0.5 compatibility and runner sequencing #67

Closed
wants to merge 28 commits into from

Conversation

wied03
Copy link

@wied03 wied03 commented Aug 14, 2015

….0.0 can be used

fixes opal/opal-rspec#27

@wied03
Copy link
Author

wied03 commented Aug 14, 2015

Thought it would be a drop in fix, but will need to look at what's failing when I can setup an environment :(

@elia
Copy link
Member

elia commented Aug 14, 2015

I'm planning to release a new version and either support both opal-rspec versions or just the latest. PR is still very appreciated 😛 (but feel free to concentrate on upgrading to rspec 3.3!)

@wied03
Copy link
Author

wied03 commented Aug 15, 2015

@elia - It would be good if I could use this now for another reason. As a result, I dug deeper here and something that's changed doesn't work at all now with the opal-rspec runner now.

It seems like it has something to do with the way things are loaded in opal-rails now. Previously, spec_builder would ensure that the Opal::RSpec::Runner.autorun method was the last method called (after all specs were loaded).

I'm not sure this new approach will work (have the runner call in the application's spec_helper file) because every spec that gets included will trigger autorun. Even using this code and opal-rspec 0.4.3, if you set a breakpoint, the runner will get messed up and not hit all the specs. Seems kind of race condition-ish, right?

I guess that brings me to 2 questions:

  1. I can see that SpecBuilder#main_code is not used anymore, which is what used to ensure that the autorun call was last call, is not called from anywhere anymore. Any particular reason?
  2. Any reason we can't move some of this code in run.html.erb out of the view and into the controller? It would be easier to test and a little cleaner.

I'm willing to help with some of this (considering now I can't move past an older opal-rails beta), just wanted to check first.

@wied03
Copy link
Author

wied03 commented Aug 15, 2015

I just tried refactoring run.html.erb to this and it ran OK w/ rspec 0.5:

<% if pattern.present? %>
  <%= link_to 'All specs', opal_spec_path %>
<% end %>

<h2>Running:</h2>
<ul>
  <% spec_files.each do |spec_file| %>
    <li>
      <% spec_file = clean_spec_path(spec_file) %>
      <%= link_to spec_file, opal_spec_path(pattern: spec_file) %>
    </li>
  <% end %>
</ul>

<%# The list of all asset dependency %>
<% all_specs = [] %>

<%# Track root assets that will need to be bootstrapped %>
<% root_assets = {} %>

<%# Collect all assets with their dependencies %>
<% builder.clean_spec_files.each do |require_path| %>
  <% asset = lookup_asset_for_path(require_path, type: :javascript) %>
  <% dependency_paths = asset.to_a.map { |a| a.logical_path } %>
  <% all_specs += dependency_paths  %>
  <% root_assets[dependency_paths.last] = require_path %>
<% end %>

<%# Add include tags and boot code, we use #uniq as we don't want to source the same asset twice %>
<% all_specs.uniq.each do |asset_path| %>
  <%- root_asset = root_assets[asset_path] -%>
  <%= javascript_include_tag asset_path, skip_opal_loader: true %>
<% end %>

<%# Boot! %>
<%= opal_tag builder.main_code %>

How does that look? I mainly just took out the load_code array and replaced with that builder.main_code echo (and also removed autorun from spec_helper)

@wied03
Copy link
Author

wied03 commented Aug 15, 2015

I also usually set config.assets.debug = false to speed up testing (and it seems like a good direction given what sprockets said - see #51). This approach sidesteps that and the browser ends up fetching them all individually.

I know it makes sense to get rid of the temp file. To make this work w/ cases where asset debug is off, what if I made a 2nd 'view' that was rendered by the same controller (HTTP request triggered from the browser rendering the 1st view). The 2nd view would be purely JS and would be a rolled up, single JS file.

I don't want to step on your toes here so I'll wait before I spend any more time on this.

@elia
Copy link
Member

elia commented Aug 15, 2015

Well what I've done there was merely to get specs passing on Travis (which had the good and unintended consequence of simplifying things a bit). The other things I'd like to keep is the ability to allow for switching test library (eg. opal-minitest).

The first good thing to do I think it's to add a spec that demonstrates the problem and have a failing branch we can work on.

Elia Schito

On 15 Aug 2015, at 03:16, Brady Wied notifications@github.com wrote:

I also usually set config.assets.debug = false to speed up testing (and it seems like a good direction given what sprockets said - see #51). This approach sidesteps that and the browser ends up fetching them all individually.

I know it makes sense to get rid of the temp file. To make this work w/ cases where asset debug is off, what if I made a 2nd 'view' that was rendered by the same controller (HTTP request triggered from the browser rendering the 1st view). The 2nd view would be purely JS and would be a rolled up, single JS file.

I don't want to step on your toes here so I'll wait before I spend any more time on this.


Reply to this email directly or view it on GitHub.

wied03 added 6 commits August 15, 2015 13:18
* Use builder's method to generate a properly sequenced 'run' command
* Make the test runner configurable (pave way for other frameworks besides RSpec)
* Remove spec related requirements from spec_builder
@wied03 wied03 reopened this Aug 15, 2015
@wied03
Copy link
Author

wied03 commented Aug 15, 2015

Maybe I didn't explain clearly. It was already failing on 0.5 w/ the specs you had in place. It's also my opinion this would fail (with the right conditions) on any opal-rspec version since the runner gets kicked off on the first require of the first spec.

The commits I just pushed on this PR sort of address the problem (and also try be less rspec coupled). I say sort of because the specs pass manually via browser on RSpec 0.5 but I can't figure out why Capybara can't see the "x passed, y failed" result. It probably has to do with the fact that test reporting is purely async now, but I'm also seeing this in the capybara-webkit logs, which implies somehow the JS is being truncated by the webkit JS engine (same result with Selenium), so the tests never run:

RuntimeError: Don't know how to parse $populate@http://127.0.0.1:42500/assets/rspec/core/metadata.self.js?body=1:102:47

I've actually discovered Chrome was working but not Firefox. So will look into that.

@elia
Copy link
Member

elia commented Aug 15, 2015

Maybe I didn't explain clearly.

It was just me replying at 2am 🌝

It probably has to do with the fact that test reporting is purely async now, but I'm also seeing this in the capybara-webkit logs

Async stuff has always been problematic, but the fact that they run locally is actually comforting :)

@wied03
Copy link
Author

wied03 commented Aug 15, 2015

Get some sleep! :)

I think some variant of this PR is good in my slightly biased opinion, but the reason the PR isn't passing is really something in opal-rspec (opal/opal-rspec#31) so I'll work on that first.

@wied03
Copy link
Author

wied03 commented Aug 16, 2015

Hopefully this is fixed by opal/opal-rspec#32 - It turns out it was not specific to async at all, it was RSpec 3.1.

@wied03
Copy link
Author

wied03 commented Aug 16, 2015

@elia - This should work AFTER opal-rspec 0.5.0beta2 is released. Because of the submodule stuff I didn't bother targeting opal-rspec/master with the 0.5 gemfile.

See the CHANGELOG, but if asset debug is disabled, it will rollup files to ease stress on the browser (might be a better approach in the future here that supports Sprockets 4 and source maps but for now this preserves what worked OK in opal-rails < 0.8).

… are changed, considering this a minor update)
Given weirdness in travis w/ webkit + qt 4.x (see thoughtbot/capybara-webkit#550 (comment)), trying selenium/firefox
@wied03
Copy link
Author

wied03 commented Aug 17, 2015

@elia sorry for the noise. Getting travis+capybara+webkit to replicate my local environment can be frustrating. Let me know if you have any concerns w/ the move to selenium-webdriver

@wied03 wied03 changed the title Make opal-rspec dependency less strict so that versions > 0.4 and < 1… Opal-rspec 0.5 compatibility and runner sequencing Aug 17, 2015
@wied03
Copy link
Author

wied03 commented Oct 20, 2015

@elia - I just tested this PR with the latest opal-rspec code and it worked OK. Just 1 minor change was needed to the engine (and associated tests) since the RSpec runner is now stock

@wied03
Copy link
Author

wied03 commented Oct 22, 2015

A little more than just minor changes once I actually started using it. If you have issues with any of this, we can discuss.

@wied03 wied03 closed this Oct 22, 2015
@wied03 wied03 reopened this Oct 22, 2015
@wied03 wied03 closed this Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

need to call autorun in last spec if using browser
2 participants