Arvados

Reviewing 9c08bbba.

First, a philosophical question: Can/should we just skip ahead to Rails 4.1? It seems like if we're going to go through the effort of a big switch, we might as well take up the latest version. Given how quickly it came out, I'm guessing it fixed some .0 release wart.

I'm not sure what shared resource client_mtx is protecting in this branch. When api_client was a shared resource (class variable), it made sure that multiple threads didn't try to update it simultaneously. Now that api_client is an instance variable, and each thread has its own instance of the client, it makes less sense to guard against simultaneous writes. I'm not fully up on how Ruby's threading works—my feeling is that the mutex should either (a) remain a class variable and enclose the body of self.new_or_current, or (b) go away. Am I missing something?

Instead of going through the whole song and dance with permit_attribute_params, can we just say ActionController::Parameters.permit_all_parameters = true somewhere in our initialization code? (Relevant API documentation)

There's trailing whitespace in arvados_api_client.rb. git diff --check can help sniff this out.

I'm consistently getting this test error from Workbench. I'm going to investigate a bit to make sure it's not a configuration issue on my end, but in case you've seen it:

  1) Error:
UsersTest#test_create_a_new_user:
Capybara::ElementNotFound: Unable to find css "tr[data-object-uuid]" with text "foo@example.com" 
    test/integration/users_test.rb:73:in `block in <class:UsersTest>'
    test/test_helper.rb:109:in `_run'

I got this warning from bundle install, did you too? Based on the message and my testing, I think this is a non-issue for us, but it might be good to document if it's expected.

themes_for_rails at /home/brett/.rvm/gems/ruby-2.1.1/bundler/gems/themes_for_rails-1fd2d7897d75 did not have a valid gemspec.
This prevents bundler from installing bins or native extensions, but that may not affect its functionality.
The validation message from Rubygems was:
  duplicate dependency on rails (= 3.0.11, development), (>= 3.0.0) use:
    add_runtime_dependency 'rails', '= 3.0.11', '>= 3.0.0'
Using themes_for_rails (0.5.1) from https://github.com/holtkampw/themes_for_rails (at 1fd2d78)

Thanks.

Brett Smith wrote:

I'm consistently getting this test error from Workbench.

While I was working on debugging it, I got a passing run. It's clear that the new user is not always on the page when we visit '/users'. I wonder if the switch to per-thread API clients has made it more likely that we'll reuse cached results, or fire a GET request before the API server has finished dealing with the post? I think this could perhaps be mitigated in the test by waiting for some acknowledgment of success after we click_button "Submit" before we move on.

Brett Smith wrote:

First, a philosophical question: Can/should we just skip ahead to Rails 4.1? It seems like if we're going to go through the effort of a big switch, we might as well take up the latest version. Given how quickly it came out, I'm guessing it fixed some .0 release wart.

I don't see why not.

With rails 4.1 we get Minitest 5, which causes our test suite to

NoMethodError: undefined method `runner=' for Minitest::Unit:Class

We can't have Minitest 4 any more:

Bundler could not find compatible versions for gem "minitest":
  In Gemfile:
    rails (~> 4.1.0) ruby depends on
      actionview (= 4.1.1) ruby depends on
        activesupport (= 4.1.1) ruby depends on
          minitest (~> 5.1) ruby

I'll get into this a bit more. Hopefully upgrading to Minitest 5 won't be that hard, and obviously we'll have to do it sooner or later.

I'm not sure what shared resource client_mtx is protecting in this branch. When api_client was a shared resource (class variable), it made sure that multiple threads didn't try to update it simultaneously. Now that api_client is an instance variable, and each thread has its own instance of the client, it makes less sense to guard against simultaneous writes. I'm not fully up on how Ruby's threading works—my feeling is that the mutex should either (a) remain a class variable and enclose the body of self.new_or_current, or (b) go away. Am I missing something?

The idea here is that an ArvadosApiClient instance should behave properly when shared by multiple threads, even though Workbench doesn't use it that way. (Looking forward to extracting this as an arvados-rails gem.)

Instead of going through the whole song and dance with permit_attribute_params, can we just say ActionController::Parameters.permit_all_parameters = true somewhere in our initialization code? (Relevant API documentation)

Tried that, but it doesn't permit nested attributes, so permit! is still necessary. Added a comment, for future readers.

There's trailing whitespace in arvados_api_client.rb. git diff --check can help sniff this out.

Fixed. (Apparently I lost my "verify on commit" setting somewhere, oops.)

I'm consistently getting this test error from Workbench. I'm going to investigate a bit to make sure it's not a configuration issue on my end, but in case you've seen it:

Doesn't look familiar, and can't seem to reproduce here. Troubling, though. Any further clues? I'll poke at it too.

I got this warning from bundle install, did you too? Based on the message and my testing, I think this is a non-issue for us, but it might be good to document if it's expected.

Yes, good point. Added a note to doc/install/install-workbench.

Now @ b9d9994

Brett Smith wrote:

While I was working on debugging it, I got a passing run. It's clear that the new user is not always on the page when we visit '/users'. I wonder if the switch to per-thread API clients has made it more likely that we'll reuse cached results, or fire a GET request before the API server has finished dealing with the post? I think this could perhaps be mitigated in the test by waiting for some acknowledgment of success after we click_button "Submit" before we move on.

Added a few wait_for_ajax in e30d714

(This has come up for me before, pre-rails4-branch: I think the trouble is "submit a form, then hit refresh, which aborts the ajax request -- sometimes before the (workbench) server hears it." Rails4, threading stuff, phase of the moon probably all affect likelihood of failure.)

Tom Clegg wrote:

With rails 4.1 we get Minitest 5, which causes our test suite to

NoMethodError: undefined method `runner=' for Minitest::Unit:Class

This comes from the very end of test/test_helper.rb, which installs a new test runner to bring the API server up and down. Looking at the Minitest 5 documentation, it looks like the new way to do it is to bring the API server up directly from test_helper.rb, then use Minitest.after_run to install a block to bring it down after tests are done.

Tom Clegg wrote:

Added a few wait_for_ajax in e30d714

This gets it passing consistently for me. I think we can remove the sleep(1) now, which seems to be attempting the same thing (I tested this way too).

Everything else looks good to me, so I think the only issue currently outstanding is how to adapt to Minitest 5.

Rails 4.1 and Minitest 5 working @ 3a696ad

(and removed some sleep(0.1) @ c818e15)

Reviewing c818e15d

Thanks for cleaning up the whole API server running class. It is a little chatty on stderr. Did you mean those to be informational messages, or are they debugging you meant to take out? I'm fine with either answer; the information seems reasonable for a test runner.

I don't feel qualified to comment on the switch from Rack to Passenger. It seems fine to me, but I don't really understand the consequences of this decision.

I got this warning while running the tests: WARN: tilt autoloading 'coffee_script' in a non thread-safe way; explicit require 'coffee_script' suggested. If one of the goals of this branch is better thread safety, perhaps it'd be worth tracking down and squashing this?

Thanks again.

Brett Smith wrote:

Thanks for cleaning up the whole API server running class. It is a little chatty on stderr. Did you mean those to be informational messages, or are they debugging you meant to take out? I'm fine with either answer; the information seems reasonable for a test runner.

Yes, that was intentional. I initially added it to debug, but then I figured it's more polite to say something about all the "starting servers, running rake tasks in other project directories" stuff happening here that one wouldn't normally expect "rake test" to do. (Better than an inexplicable list of "create table" operations coming from a project that has no database?)

I don't feel qualified to comment on the switch from Rack to Passenger. It seems fine to me, but I don't really understand the consequences of this decision.

1. Passenger is [at least very close to] what we use/recommend for a real server. For example, we had a "HEAD an GET must not have a request body" problem recently which Webrick doesn't care about but passenger/nginx does. (If running real server software is this easy, why bother running non-real server software?)
2. Passenger can serve websockets. We don't have tests that rely on websockets, but we will soon. (Enabling websockets should amount to turning on "ARVADOS_WEBSOCKETS=true" before starting.)

I got this warning while running the tests: WARN: tilt autoloading 'coffee_script' in a non thread-safe way; explicit require 'coffee_script' suggested. If one of the goals of this branch is better thread safety, perhaps it'd be worth tracking down and squashing this?

Hm, I saw that one too, but I did something to Gemfile and it stopped happening. Now it sounds like that was just a coincidence and there's a real race condition in there. I'll look further.

Thanks

Tom Clegg wrote:

Hm, I saw that one too, but I did something to Gemfile and it stopped happening. Now it sounds like that was just a coincidence and there's a real race condition in there. I'll look further.

Clearing the cache seems to produce the warning reliably:

rm -r tmp/cache/*; RAILS_ENV=test bundle exec rake test

This should fix the autoload problem/warning: 8182d6e

I pulled 8182d6ed, cleared my cache, and reran the tests. They all passed without the warning this time. Thanks for digging into that. I think this branch is good to merge now.