mountetna / magma

Data server with friendly data loaders
GNU General Public License v2.0
5 stars 2 forks source link

Update retrieve TSV spec #155

Closed alimi closed 4 years ago

alimi commented 4 years ago

This fixes a flaky failure that happened after https://github.com/mountetna/magma/pull/151.

https://github.com/mountetna/magma/runs/721348936

I'm assuming the TSV header order doesn't matter, so I used the match_array helper to verify the header elements are present without worrying about the order. If the order matters, we can try something else.

graft commented 4 years ago

In this case there are a bunch of things going on. The request to /retrieve includes attribute_names, which is an array of names, i.e. there is an ordering specified. In a TSV i'd expect the columns to come back in that order. Instead magma seems to just return in the order of model.attributes (lib/magma/retrieval.rb:29). The model ordering should be in the order they were loaded.

However, i think now loading from the db, that ordering has become unspecified. Previously the test coincidentally requested attribute names in the right order. If it had requested, and expected, in an order other than the model-order, it would have been failing already.

Under these circumstances I think the correct way to fix the test would be to sort the attributes in Magma::Retrieval correctly. It would also be nice to have a consistent ordering of model.attributes, but I don't think that's the issue here.

graft commented 4 years ago

We should merge this, but make an issue for consistent attribute ordering in retrieve.

alimi commented 4 years ago

Gotcha. I gave ordering Magma::Retrieval#attributes a shot in https://github.com/mountetna/magma/pull/155/commits/3bed1265aeb1ca80df5f6dddd36a88eba5b9fed9.

alimi commented 4 years ago

We have another unrelated ordering failure:

https://github.com/mountetna/magma/blob/c93811f7eeb106672a83edbc1aa5d2b9315642c8/spec/retrieve_spec.rb#L237

   1) RetrieveController tables retrieves table associations
     Failure/Error: expect(models[:labor][:documents][:'Lernean Hydra'][:prize]).to eq(hydra_prizes.map(&:id))

       expected: [111, 112, 113]
            got: [113, 111, 112]

https://github.com/mountetna/magma/pull/155/checks?check_run_id=727672362

Does the ordering matter here @graft?

graft commented 4 years ago

:shrug: on how i'm supposed to reply to these conversations in-line - err, that new spec doesn't seem to matter? That is, if it is just checking the keys of a hash I can't expect that to preserve ordering in the way that a TSV column header would.

alimi commented 4 years ago

Heh, yeah it's tricky responding to comments. You can Quote reply to a comment. It's in the ... menu.

image

that new spec doesn't seem to matter? That is, if it is just checking the keys of a hash I can't expect that to preserve ordering in the way that a TSV column header would.

👍🏾 I'll update the assertion.

alimi commented 4 years ago

Ok, https://github.com/mountetna/magma/pull/155/commits/77aeb61c1d839ee9d48dae40acc117415b6f9a06 looks to have fixed more flakiness in spec/retrieve_spec.rb.

graft commented 4 years ago

Looks good to merge, let's :ship: so our tests aren't flaky!