Fix #77

[ArnyminerZ]

Improved FixInvalidDayOffsetPreprocessor, and updated tests to make sure the fix is working.

Closes #77

[ArnyminerZ]

@sunkup There's FixInvalidDayOffsetPreprocessorTest and FixInvalidUtcOffsetPreprocessorTest, which I understand is what you mean to have. ICalPreprocessorTest tests the preprocessor with complete icals, the other ones test smaller features manually

[rfc2822]

I have added the mockk library to allow mocking and adapted test tests. Now ICalPreprocessorTest can test what actually happens, namely that ICalPreprocessor.preprocessStream calls FixInvalidDayOffsetPreprocessor.preprocess(…) and FixInvalidUtcOffsetPreprocessor.preprocess(…).

So now we have should have tests that test what every method actually does.

[ArnyminerZ]

So... All tests are good now?

[rfc2822]

As far as I see in #77, the problem was with TRIGGER:-P5DT. There should be a test for this class of problems in FixInvalidDayOffsetPreprocessorTest that fails without the changes made in this PR and then succeeds.

[ArnyminerZ]

Well, this is tested: https://github.com/bitfireAT/ical4android/blob/cf5c663175d16d75cddc614767458de5d92da9e4/src/test/java/at/bitfire/ical4android/validation/FixInvalidDayOffsetPreprocessorTest.kt#L27-L34

It tests for TRIGGER:-P2DT instead of TRIGGER:-P5DT, but that shouldn't be an issue. I can add TRIGGER:-P2DT as well, but I think it's just repetitive. What we can do is passing the problematic ics (see), and check if it processes correctly.

[rfc2822]

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?

[ArnyminerZ]

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?

[rfc2822]

I think there were actually no failing tests

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.

it was a problem reported from a user in a specific ical, so maybe testing against that ical is not a bad idea

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

• fails without your changes in the preprocessor,
• but passes with your changes.

[ArnyminerZ]

I'd not test against the whole iCal, but the specific line

@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 https://github.com/bitfireAT/ical4android/blob/cf5c663175d16d75cddc614767458de5d92da9e4/src/test/java/at/bitfire/ical4android/validation/FixInvalidDayOffsetPreprocessorTest.kt#L12-L18 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:

Text cannot be parsed to a Duration
java.time.format.DateTimeParseException: Text cannot be parsed to a Duration
    at java.base/java.time.Duration.parse(Duration.java:417)
    at at.bitfire.ical4android.validation.FixInvalidDayOffsetPreprocessorTest.fixStringAndAssert(FixInvalidDayOffsetPreprocessorTest.kt:16)
    at at.bitfire.ical4android.validation.FixInvalidDayOffsetPreprocessorTest.test_FixString_DayOffsetFromMultiple_Mixed(FixInvalidDayOffsetPreprocessorTest.kt:58)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
    at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
    at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:110)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
    at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38)
    at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62)
    at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
    at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
    at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
    at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
    at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
    at org.gradle.api.internal.tasks.testing.worker.TestWorker$2.run(TestWorker.java:176)
    at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
    at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
    at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
    at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
    at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
    at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
    at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
    at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)

but passes with the current fix PR.

See the pushed changes