Adding back references tests. #104
Conversation
| * Check that we can request a sequence of bases from | ||
| * {@link org.ga4gh.ctk.transport.protocols.Client.References#getReferenceBases(String, | ||
| * ListReferenceBasesRequest)} | ||
| * using a size twice as large as the full sequence, and receive the full sequence. |
There was a problem hiding this comment.
This doc does not appear to agree with the actual behavior.
There was a problem hiding this comment.
You're right, I fixed the comment.
| */ | ||
| @Test | ||
| public void checkPagingByOneChunkThroughReferenceSets() throws AvroRemoteException { | ||
|
|
There was a problem hiding this comment.
This is unlikely to fail in practise, but the server is not required to satisfy the pageSize requests. It's perfectly entitled to send back the results in pages of 1, if it so desires. This won't fail now because we only have a handful of objects, but if there was 1000 reference sets, then some servers would send back the results in chunks smaller than 1000. As such, I think this is a bit problematic and should possibly be removed.
There was a problem hiding this comment.
@jeromekelleher to be precise, my understanding is that a compliant server should never send back more objects per request than specified by pageSize (it can certainly send less).
There was a problem hiding this comment.
Fair enough @macieksmuga --- we must have 0 < returnedPageSize <= requestedPageSize.
There was a problem hiding this comment.
I believe all the paging tests now check this.
|
This looks great, thanks @hjellinek and @macieksmuga. The code is easy to read and the tests are very nice. I agree with all the comments @calbach makes above. I'm slightly worried about these paging tests though. I think we should aim to centralise the paging code and concentrate on testing the stream of objects we get back. This is the interesting bit. |
|
Any updates on this @hjellinek? |
|
With the pull of @hjellinek's latest code, are people's issues answered? Can we get the +1's to make this mergeable? |
|
I still see a bunch of unaddressed comments here (mostly from @jeromekelleher), I also just noticed one more issue. |
| String expectedSequence) | ||
| throws AvroRemoteException { | ||
|
|
||
| final String refId = Utils.getValidReferenceId(client); |
There was a problem hiding this comment.
This approach only works because there's exactly one Reference in the system. How I expected this to work ideally, is that a helper would perform a lookup by assembly ID to find your known reference set, then do a lookup by reference set ID + reference name to get a specific reference ID.
There was a problem hiding this comment.
Good one. There's a lot of room for new references-related tests, and the surest route to that would be to add more references to the compliance data set. It's difficult to write tests with generality or confidence when you know there's only a single instance of the object under test.
Once there are multiple references, I expect things like Utils.getValidReferenceId(client) to go away in most tests in favor of the approach you outlined.
There was a problem hiding this comment.
I had missed this batch of @jeromekelleher's comments, but believe I have now addressed them.
There was a problem hiding this comment.
The test has a hidden dependency on there being exactly one reference, which I consider to be an issue now - not only later, once more references are added. Ideally this would be fixed before the test is submitted, but if not there should at least be a TODO here (I wouldn't block my +1 on it being fixed here).
|
OK, happy to +1 (once #116 is merged?). We could do with some more refactoring to take out the reliance of looping over pageTokens, but this is a good start. |
|
Thanks, @jeromekelleher. I don't know that there's anything blocking #116, so I hope this can happen soon. |
|
I think you can just update this PR by pushing to it directly @hjellinek. Having PRs against PRs seems like overkill... |
|
+1, though I second the need for an eventual second pass on pagination here. There are still several places where the logic is not correct, though is likely to pass on most servers (e.g. all the tests which check pageSize * 2). It may make sense to pair a PR of pagination cleanup/fixes with some additional pagination documentation in the schemas branch. |
|
Also +1, and completely agree with @calbach's comments. |
|
I think we're ready to merge here. One question though: are we going to start squashing commits on PRs like this? Here we have some fairly boring updates and merge noise which would be much cleaner as a single commit. I'd prefer a clean history, but I guess it doesn't matter so much here in the compliance tests since we're not going to be making bugfix releases on older versions (probably??) so there won't be much cherry picking going on. |
|
I'll be happy to squish the commits here and in the other parallel PR's |
|
Can we squash please @macieksmuga? Ready to merge otherwise I think. |
Modified the Reference and ReferenceSet paging tests to take into account the possibility that a request for "all" objects in one chunk may not actually return all of them. Also submitted related issue ga4gh/ga4gh-schemas#430.
macieksmuga
force-pushed
the
references_tests
branch
from
9ca4fd7
to
7b228d7
Compare
|
Squashed commits & rebased, @jeromekelleher. Should be ready to merge. |
|
OK, we've got the requisite +1-age and a nice clean commit history, so I'm merging. Thanks @macieksmuga and @hjellinek! |
Adding back references tests.
No description provided.