Arvados

I'm looking at c52a78b3, and I'm calling it now: Tom wins this sprint review. No other demo could possibly compare to this branch for raw eye candy. "Oh, we can run Crunch jobs in Docker containers now? That's nice. Me? Oh, I just MADE THE WORKBENCH INTERFACE GLORIOUS. BEHOLD my expansive collection of beautiful screenshots!" I can hear the business team ohh-ing and ahh-ing already.

Seriously, this is very impressive work. Kudos. My comments feel even more nit-pickier than usual by comparison (and really, they're mostly questions for understanding rather than suggestions for improvements), but here goes anyway:

• ActionsController calls #send(sym.to_s). Sending symbols is allowed (and I think preferred), so you can omit .to_s.
• FoldersController#index returns @object. What is that for index methods?
• The first folder integration test has no assertion. Could you assert find('.panel').has_text?('I just edited this.')?
• Maybe related, I'm curious why you removed some of the intermediate assertions in the existing integration tests.
• I'm not wild about the cloud icon for virtual machines in the navigation sidebar. Virtual machines may be in the cloud, but they are not themselves the cloud. Is there a generic computer/workstation icon? A work desk might make sense, too, since the virtual machine is where people work on Crunch scripts.

Thanks much.

Brett Smith wrote:

Seriously, this is very impressive work. Kudos. My comments feel even more nit-pickier than usual by comparison (and really, they're mostly questions for understanding rather than suggestions for improvements), but here goes anyway:

Thanks!

• ActionsController calls #send(sym.to_s). Sending symbols is allowed (and I think preferred), so you can omit .to_s.

Indeed, fixed.

• FoldersController#index returns @object. What is that for index methods?

Hm, not actually sure why that got there. Removed.

• The first folder integration test has no assertion. Could you assert find('.panel').has_text?('I just edited this.')?

Fixed. (I had commented it out pending discovering the magic incantation to reconcile Capybara with x-editable. "visit current_path" isn't perfect but better than nothing.)

• Maybe related, I'm curious why you removed some of the intermediate assertions in the existing integration tests.

Some of the tests changed from "click the uuid link" to "click the Show button on the row with this uuid". While I was updating them, noticing find() errors out if the links don't exist, I figured the extra assert before find().click doesn't serve any purpose (unless it has a helpful error message).

• I'm not wild about the cloud icon for virtual machines in the navigation sidebar. Virtual machines may be in the cloud, but they are not themselves the cloud. Is there a generic computer/workstation icon? A work desk might make sense, too, since the virtual machine is where people work on Crunch scripts.

Yeah, I'm not super keen on it either. I've changed it to the "terminal" icon, see how that goes. See http://fortawesome.github.io/Font-Awesome/icons/

Latest: 16c58b7

Brett Smith wrote:

On 16c58b75

You say below that find errors out if it doesn't find what it's looking for. So won't this always either pass or raise an exception? Or does that only apply when you try to click the result?

That's true, the word "assert" doesn't actually do anything because failure results in an exception.

• Tried wrapping in "assert_nothing_raised", but that somehow causes find() to always fail.
• Tried using the "message" option to find(), but that feature has been broken/removed from Capybara.
• Added a comment (in a848b6e) explaining that find() raises an exception if the tested behavior fails.
• Added a find?() method (in 32ca1dc) to use with assert.

+    assert(find?('.panel', text: 'I just edited this.'),
+           "Description update did not survive page refresh")

Totally on board with improving our testing standards/patterns. In particular, valuable to make more clear in the tests which parts are "setting up the test" (rearrange as needed) and which parts are "behavior actually being tested" (when you touch this code, be careful that you are still testing this behavior!).