Brett208
commented
3 years ago Oh, yeah good point about the slice starting place mattering... Funny that I mentioned test driven development but didn't use it and then wrote a test that didn't actually properly test the problem.
I would agree the test code could use some improvement. I suppose if the behaviour is purposefully different for different implementations of the abstract class, it should be tested individually outside of the core test cases... I guess since the slice reader could take a slice of something in memory or from file, it may have different boundary conditions to test against?
Related: #388
I updated the unit tests so they would actually catch the bug that was fixed in #388.
After pondering how to write a test for the current bug, I was not able to think of a way that could be easily inserted into existing unit test code. The comparison that leads to an exception basically goes the wrong way, where the old buggy code would not throw an exception when it should have. The only way to test the current code would be to check if it throws an exception when it should, however, not all stream classes that derive from
BidirectionalReaderwillthrowif you seek past the end. In particular,FileReaderwill not throw, so adding athrowcheck to the test will fail for that class.I find the current stream tests somewhat hard to navigate and reason about. Maybe we should revisit the organization of that code at some point. I remember certain Google Test concepts were pretty new at that point, and we were still experimenting. Perhaps it would be clearer if we revisited the design now.