Arvados

I'm having trouble following the original failing test. It starts out by assigning a static test_collection[:manifest_text]. It creates test_collection, then re-shows it, and compares test_collection against that response. Yet somehow permission signatures appear in test_collection, based on the output in this ticket. As best I can tell, the only way this can happen is if the create method modifies test_collection in place.

Am I missing something? If not, I'm concerned that the bigger issue here is not the clock, but instead the test's reliance on these side effects in the create method. Aside from bad timing, the test could also fail if somone changes this implementation detail. I think a better improvement would be to modify the test itself so that it only checks or relies on behavior that we actually care about—that would both fix the immediate problem and prevent future trouble.

But if I've overlooked something and misunderstood, do let me know.

Changes @ 488a811:

You are correct that the controller is mutating the collection that we pass to it. Ew!

I've rewritten the "should create" test (and the "multiple locators per line" test which appears to have been a victim of the same bug) to guarantee via marshalling and unmarshalling that the arguments to post :create are distinct from the source test data. Please feel free to double check these assumptions.

Injecting a fake clock is no longer necessary, so I've taken out that code. It would be good to be able to mock stuff like this in the future, but there are surely better ways of doing it.

Review from master: master...origin/2857-collection-tests

Tim Pierce wrote:

I've rewritten the "should create" test (and the "multiple locators per line" test which appears to have been a victim of the same bug) to guarantee via marshalling and unmarshalling that the arguments to post :create are distinct from the source test data. Please feel free to double check these assumptions.

Injecting a fake clock is no longer necessary, so I've taken out that code. It would be good to be able to mock stuff like this in the future, but there are surely better ways of doing it.

On the whole, this approach seems a lot more sensible to me. Thanks for taking the time to dig in and rework it. My comments now are much smaller:

• I know this might not be your code, but while we're cleaning up in here, I doubt that assert_nil assigns(:objects) is doing much interesting. Only index methods would normally assign this, and I expect the rest of the stack would ignore it if create did assign it. If you understand its intent better, maybe there's a better assertion that could go here? Otherwise, I think it should come out.
• Why are _ and - in the signature-stripping regexp? This question is mostly for my own edification—I'm concerned that I need to go back and fix my own signature-parsing code. :)

Once you deal with the odd assertion I think you're good to merge. Thanks again.

Brett Smith wrote:

• I know this might not be your code, but while we're cleaning up in here, I doubt that assert_nil assigns(:objects) is doing much interesting. Only index methods would normally assign this, and I expect the rest of the stack would ignore it if create did assign it. If you understand its intent better, maybe there's a better assertion that could go here? Otherwise, I think it should come out.

Well, it's not my code and I'm not sure I do understand its intent better. However! :-) My understanding here is that this confirms that Collections.find_object_by_uuid was not called successfully, i.e. that the system did not find any existing collection in the database by that UUID when we attempted to create one, and that the subsequent assert_not_nil assigns(:objects) after performing GET confirms that it did find one. That makes sense to me -- does it look different to you?

I'm going to go ahead and merge and close this ticket, since this particular code isn't affected by this issue, but let's make sure to close the loop and confirm whether this assertion actually expresses what we think it does.

• Why are _ and - in the signature-stripping regexp? This question is mostly for my own edification—I'm concerned that I need to go back and fix my own signature-parsing code. :)

Tom and I (mostly me) decided in #2755 that a blob locator hint may contain alphanumerics, "@", "_" and "-". We do not currently have any hints that use _ or - that I know of, but I threw those in on the basis that they're very often considered valid identifier or word characters, and I would personally be surprised to generate a locator with those characters and find that it wasn't parsed correctly.