Arvados

At 3374f67, I have one bug report and one test comment.

The bug report: the API server version reported in the discovery document can include a "locally modified" string, just like the Workbench version. When it does, the current code generates a bad GitHub link. You might fix it by having separate api_version and api_version_text methods as with Workbench; the version method can return what's in version_text until the first non-hex character. But that's just one suggestion; I'm sure there are other options.

Which brings me to my test comment: because this story is very specific about where the links should go, you could make the tests stronger by checking that the links actually go where we want. Something like:

wb_version_href = "https://github.com/curoverse/aravados/#{Rails.configuration.source_version}/apps/workbench" 
refute_empty(all("a").select { |a| a[:href] == wb_version_href })
refute_empty(all("a").select { |a| a[:href] =~ /https:\/\/github.com\/curoverse\/aravados\/[0-9a-f]+\/services\/api\// })

This would also improve test isolation (though it's still not perfect) by eliminating the dependency on VersionHelper. Those could be tested independently in dedicated helper tests. Those would be nice and easy to write because you can just set the configuration values to whatever you need, then check the string results directly.

Thanks.

Brett Smith wrote:

At 3374f67, I have one bug report and one test comment.

The bug report: the API server version reported in the discovery document can include a "locally modified" string, just like the Workbench version. When it does, the current code generates a bad GitHub link. You might fix it by having separate api_version and api_version_text methods as with Workbench; the version method can return what's in version_text until the first non-hex character. But that's just one suggestion; I'm sure there are other options.

Good call, and I'm fine with that suggestion. Added at b629c24.

Which brings me to my test comment: because this story is very specific about where the links should go, you could make the tests stronger by checking that the links actually go where we want. Something like:

[...]

As background: this story is only specific about where the links should go because, uh, I wrote that specification to describe what I was doing. When I inherited the story it had no description at all, and I read #4175 to get more context about what we wanted it to do here. The github links that I added were my best guess as to what would be useful for someone who needs to investigate a version-related bug issue.

(I didn't mean to slip one over anybody, just keep velocity high -- I intended to ask explicitly whether this is the right choice when I requested the code review, I just forgot.)

That said, I do agree that it makes more sense in the end for the integration test not to be dependent on the VersionHelper. Updated tests at b0001b6 to look for link targets that refer to Github links into the repository history.

Tim Pierce wrote:

As background: this story is only specific about where the links should go because, uh, I wrote that specification to describe what I was doing. When I inherited the story it had no description at all, and I read #4175 to get more context about what we wanted it to do here. The github links that I added were my best guess as to what would be useful for someone who needs to investigate a version-related bug issue.

Thanks for pointing that out. In fairness, the subject at least mentions a GitHub link, so I think you're pretty well in the clear here.

My one nit for b0001b6d is that it might be nice to end the text regexps with /?, so tests don't start failing if a trailing slash starts getting added somewhere. This looks good to merge, thanks.

How about https://arvados.org/projects/arvados/repository/changes?rev=86f994b ...?