-
Notifications
You must be signed in to change notification settings - Fork 66
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
Conversation
Thought it would be a drop in fix, but will need to look at what's failing when I can setup an environment :( |
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!) |
@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:
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. |
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) |
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. |
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
|
* 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
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:
I've actually discovered Chrome was working but not Firefox. So will look into that. |
It was just me replying at 2am 🌝
Async stuff has always been problematic, but the fact that they run locally is actually comforting :) |
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. |
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. |
* Don't fail the asset debug example on our new env (would be pointless)
* Skip source map tests when asset debug is disabled
@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
@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 |
…ot in asset debug mode, we don't need this anymore
@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 |
A little more than just minor changes once I actually started using it. If you have issues with any of this, we can discuss. |
….0.0 can be used
fixes opal/opal-rspec#27