Improves the factoring of IonReaderContinuableTopLevelBinaryTest.java. #607
Conversation
| static ExpectationProvider<IonReaderContinuableTopLevelBinary> next(IonType expectedType) { | ||
| return consumer -> consumer.accept(new Expectation<>( | ||
| String.format("next(%s)", expectedType), | ||
| reader -> assertEquals(expectedType, reader.next())) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should this subsume the type check as well? Should it not always be the case that whatever type is yielded by reader.next() should also be then yielded by reader.getType()? Then you wouldn't have to repeat yourself below with e.g. next(IonType.LIST), type(IonType.LIST), ...
There was a problem hiding this comment.
Yes, I'll add the reader.getType() assertion here and remove any usages of type(...) that immediately follow a call to next(...). type(...) is still used in several cases where reader.next() is not desired, such as when asserting that stepping into / out of a container clears the type or that parsing a scalar value does not clear the type.
| next(IonType.SYMBOL), stringValue("abc"), | ||
| next(IonType.SYMBOL), stringValue("def"), | ||
| next(IonType.SYMBOL), stringValue("purple"), |
There was a problem hiding this comment.
Why not use symbolValue(...)? Same elsewhere.
There was a problem hiding this comment.
stringValue and symbolValue test different reader APIs. IonReader.stringValue is applicable to both symbols and strings, so we test it for both. IonReader.symbolValue is applicable only to symbols.
| expectOversized(0); | ||
| nextExpect(IonType.STRING); | ||
| assertSequence(next(IonType.STRING)); | ||
| expectOversized(0); |
There was a problem hiding this comment.
Can the oversized counter can be decremented? Otherwise, is it just as strong to assert 0 at the end so that you don't have to break this into two sequences?
There was a problem hiding this comment.
Yes, I'm guessing I added assertions at several points during development while trying to get this to pass, and it survived the restructuring. I'll simplify into one assertion sequence with a single assertion at the end.
tgregg
force-pushed
the
reentrant-reader-base-pr33
branch
from
c101027 to
e01b348
Compare
Issue #, if available:
Addresses #428 (along with #604 and #605).
Description of changes:
Implements the technique agreed upon in this template and applied to IonCursorBinaryTest, IonReaderContinuableCoreBinaryTest, and IonReaderContinuableApplicationBinaryTest in #604.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.