Fix #77 #86
Fix #77 #86
Conversation
Signed-off-by: Arnau Mora <arnyminer.z@gmail.com>
Signed-off-by: Arnau Mora <arnyminer.z@gmail.com>
Signed-off-by: Arnau Mora <arnyminer.z@gmail.com>
There was a problem hiding this comment.
I'd have some comments about the tests, before looking at the actual implementation.
The test in ICalPreprocessorTest is kind of an integration test, because it tests ICalPreprocessor and not FixInvalidDayOffsetPreprocessor, which is the one we would actually want to test for given implementation of fixString().
I suggest to test every aspect of fixString() with multiple, small, descriptive unit tests in "FixInvalidDayOffsetPreprocessorTest" and drop "testFixInvalidUtcOffset" and "testFixInvalidDuration" in ICalPreprocessorTest.
Everything that is rewritten by the preprocessor should be tested by a small test method with a clear name (usually one assert per @test method).
|
@sunkup There's |
|
I have added the mockk library to allow mocking and adapted test tests. Now So now we have should have tests that test what every method actually does. |
|
So... All tests are good now? |
|
As far as I see in #77, the problem was with |
|
Well, this is tested: It tests for |
|
No, that's ok. I just wonder whether this test was added in this PR? Because I don't see it in the summarized changes of this PR or in the commits. The important point is that this test should fail before your changes and then succeed after your changes. Which test fails without your changes? |
|
I think there were actually no failing tests, it was a problem reported from a user in a specific ical, so maybe testing against that ical is not a bad idea, to make sure that it's not introduced in the future, and maybe keep a folder so we can keep adding specific calendars that cause issues? |
Signed-off-by: Arnau Mora <arnyminer.z@gmail.com>
This is what I meant: in the spirit of test-driven development (which we don't use, but it's a good idea to think like this), there should be a test that fails without the changes.
I'd not test against the whole iCal, but the specific line. So there should be a test with for instance one TRIGGER (or whatever the problem was, taken from the user'siCal) that
|
@rfc2822 The issue was in fact that we were testing with specific cases, so the error was not being tested. The error was caused by multiple occurrences of this invalid duration on the same calendar. Since we were only testing one case, this was not an error, until a calendar with multiple bad durations is imported. So maybe it's better to have another test case, as stated by @sunkup, to keep them simple, something like: @Test
fun test_FixString_DayOffsetFromMultiple_Invalid() {
fixStringAndAssert("DURATION:-P1D\nTRIGGER:-P2D", "DURATION:-PT1D\nTRIGGER:-PT2D")
fixStringAndAssert("DURATION:-P1D\nTRIGGER:-P2D", "DURATION:-P1DT\nTRIGGER:-P2DT")
}
@Test
fun test_FixString_DayOffsetFromMultiple_Valid() {
fixStringAndAssert("DURATION:-PT12H\nTRIGGER:-PT12H", "DURATION:-PT12H\nTRIGGER:-PT12H")
}
@Test
fun test_FixString_DayOffsetFromMultiple_Mixed() {
fixStringAndAssert("DURATION:-P1D\nDURATION:-PT12H\nTRIGGER:-P2D", "DURATION:-PT1D\nDURATION:-PT12H\nTRIGGER:-PT2D")
fixStringAndAssert("DURATION:-P1D\nDURATION:-PT12H\nTRIGGER:-P2D", "DURATION:-P1DT\nDURATION:-PT12H\nTRIGGER:-P2DT")
}Which tests for multiple bad durations. This also needs to replace the code in `` from with private fun fixStringAndAssert(expected: String, string: String) {
val fixed = FixInvalidDayOffsetPreprocessor.fixString(string)
for (line in fixed.split('\n')) {
Duration.parse(line.substring(line.indexOf(':') + 1))
}
assertEquals(expected, fixed)
}With this test, the code before the changes throws: but passes with the current fix PR. See the pushed changes |
Signed-off-by: Arnau Mora <arnyminer.z@gmail.com>
Signed-off-by: Arnau Mora <arnyminer.z@gmail.com>
There was a problem hiding this comment.
Looks really good now. Much better. Although I do not like to deviate from the AAA pattern, especially for unit tests, I guess it can't really be helped in this case. So tried to make it a bit more clear what happens using comments.
Go merge, from me :)
rfc2822
left a comment
rfc2822
left a comment
There was a problem hiding this comment.
Thanks @ArnyminerZ @sunkup 👍🏻 😃
3 participants
Improved
FixInvalidDayOffsetPreprocessor, and updated tests to make sure the fix is working.Closes #77