search

bitfireAT / ical4android

Allows usage of iCalendar files with the Android calendar provider
GNU General Public License v3.0
19 stars 10 forks source link

recurrenceSetsToAndroidString, androidStringToRecurrenceSet: handle multiple entries separated by newline #104

Closed ArnyminerZ closed 1 year ago

ArnyminerZ commented 1 year ago

Replaces #92. Closes #30

ArnyminerZ commented 1 year ago

@rfc2822 The test is failing on API 31, but passes on 33 🤔 at least on my computer.

Locally I'm getting this error:

java.lang.AssertionError: expected:<19160501T000000> but was:<19160501T010000>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.junit.Assert.assertEquals(Assert.java:146)
at at.bitfire.ical4android.ICalendarTest.testMinifyVTimezone_keepFutureObservances(ICalendarTest.kt:135)

Can you try locally and see what you get?

rfc2822 commented 1 year ago

@rfc2822 The test is failing on API 31, but passes on 33 thinking at least on my computer.

I get the same error on Android 12 emulator (with Google services):

java.lang.AssertionError: expected:<3> but was:<0>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.junit.Assert.assertEquals(Assert.java:146)
at at.bitfire.ical4android.AospTest.testRdatesWithDifferentTimezones(AospTest.kt:125)

With TZ Europe/Vienna, if that makes a difference.

Locally I'm getting this error:

java.lang.AssertionError: expected:<19160501T000000> but was:<19160501T010000>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.junit.Assert.assertEquals(Assert.java:146)
at at.bitfire.ical4android.ICalendarTest.testMinifyVTimezone_keepFutureObservances(ICalendarTest.kt:135)

Can you try locally and see what you get?

I don't have that error here. I have tried with various time zones, but my default time zone in the emulator is Europe/Vienna. Which system TZ are you using?

ArnyminerZ commented 1 year ago

Okay so the error:

at.bitfire.ical4android.AospTest > testRdatesWithDifferentTimezones[Pixel_3a_API_31(AVD) - 12] FAILED 
    java.lang.AssertionError: expected:<3> but was:<0>
    at org.junit.Assert.fail(Assert.java:89)

is only thrown for me when the timezone is not Austria, which is the one set in

put(CalendarContract.Events.RDATE, "Europe/Vienna;20230413T095049Z,20230414T000000\nEurope/London;20230414T000000")

London does still fail whatsoever.

So there must still be something wrong around, I'll keep updates here.

Edit: Austria also fails, it just didn't for some reason the first time

ArnyminerZ commented 1 year ago

Okay, when the app is first installed, test passes successfully. If you run it multiple times, everything is fine. However, if timezone is changed without uninstalling the app (package at.bitfire.ical4android.test), test fails. If you go back to the initial timezone, test still fails.

To sum up, to reproduce:

  1. Uninstall the test app (adb uninstall at.bitfire.ical4android.test)
  2. Run tests (only AospTest may be run to be quicker) ✅ Tests are passing
  3. Run tests again ✅ Tests are passing
  4. Change timezone
  5. Run tests ❌ Tests fail
  6. Go back to the initial timezone
  7. Run tests ❌ Tests fail
  8. Run test again ✅ Tests are passing

Note: equivalent timezones (e.g. Madrid-Vienna) still fail

Edit: if tests are run, and then timezone is changed and reversed, without running tests again, and then tests are run. Tests still fail. So Android is surely messing something up with this timezone changes...

ArnyminerZ commented 1 year ago

Logs for success:

TestRunner: started: testRdatesWithDifferentTimezones(at.bitfire.ical4android.AospTest)
GrantPermissionCallable: Permission: android.permission.WRITE_CALENDAR is already granted!
GrantPermissionCallable: Permission: android.permission.READ_CALENDAR is already granted!
A       : NGA disabled due to not being eligible.
ContentResolver: Failed to get type for: content://com.android.calendar (Unknown URL content://com.android.calendar)
ContentResolver: Failed to get type for: content://com.android.calendar (Unknown URL content://com.android.calendar)
TestRunner: finished: testRdatesWithDifferentTimezones(at.bitfire.ical4android.AospTest)

Logs for failure:

TestRunner: started: testRdatesWithDifferentTimezones(at.bitfire.ical4android.AospTest)
      : NGA disabled due to not being eligible.
GrantPermissionCallable: Permission: android.permission.WRITE_CALENDAR is already granted!
GrantPermissionCallable: Permission: android.permission.READ_CALENDAR is already granted!
ContentResolver: Failed to get type for: content://com.android.calendar (Unknown URL content://com.android.calendar)
ContentResolver: Failed to get type for: content://com.android.calendar (Unknown URL content://com.android.calendar)
TestRunner: failed: testRdatesWithDifferentTimezones(at.bitfire.ical4android.AospTest)
TestRunner: ----- begin exception -----
TestRunner: java.lang.AssertionError: expected:<3> but was:<0>
TestRunner:     at org.junit.Assert.fail(Assert.java:89)
TestRunner:     at org.junit.Assert.failNotEquals(Assert.java:835)
TestRunner:     at org.junit.Assert.assertEquals(Assert.java:120)
TestRunner:     at org.junit.Assert.assertEquals(Assert.java:146)
TestRunner:     at at.bitfire.ical4android.AospTest.testRdatesWithDifferentTimezones(AospTest.kt:126)
TestRunner:     at java.lang.reflect.Method.invoke(Native Method)
TestRunner:     at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
...
TestRunner:     at org.junit.runners.Suite.runChild(Suite.java:128)
TestRunner:     at org.junit.runners.Suite.runChild(Suite.java:27)
TestRunner:     at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
TestRunner:     at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
TestRunner:     at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
TestRunner:     at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
TestRunner:     at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
TestRunner:     at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
TestRunner:     at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
TestRunner:     at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
TestRunner:     at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
TestRunner:     at androidx.test.internal.runner.TestExecutor.execute(TestExecutor.java:67)
TestRunner:     at androidx.test.internal.runner.TestExecutor.execute(TestExecutor.java:58)
TestRunner:     at androidx.test.runner.AndroidJUnitRunner.onStart(AndroidJUnitRunner.java:446)
TestRunner:     at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2248)
TestRunner: ----- end exception -----
TestRunner: finished: testRdatesWithDifferentTimezones(at.bitfire.ical4android.AospTest)
rfc2822 commented 1 year ago

Just adding @sunkup, we already had encountered problems with such flaky tests. Maybe he has an idea too :)

sunkup commented 1 year ago

Just adding @sunkup, we already had encountered problems with such flaky tests. Maybe he has an idea too :)

If I recall correctly we had it working for 2 weeks or so after using the InitCalendarProvideRule. Then the flaky tests came back :/

Not sure if this could help here though

rfc2822 commented 1 year ago

New finding: The problem can be reproduced in the emulator when running adb shell pm clear com.android.providers.calendar before the tests.

It's the same problem as we had earlier…

rfc2822 commented 1 year ago

Hopefully the test now works reliable with the InitCalendarProviderRule

ArnyminerZ commented 1 year ago

Maybe we should really wait until the issue gets worse, or when ical4j 4.0 is stable. A LOT of things will change, and maybe it will be simpler to implement afterwards. I've been digging a bit on ical4j 4.0, and since it uses Java 8's time objects, timezones will be much simpler to use, and I hope we will start having less issues with that. However, migration will be a long process, since we really have to change almost the whole ical4android library, so maybe we want to hotfix this before...

rfc2822 commented 1 year ago

Agreed. I'll close it for now as "wontfix" until it pops up again (and we hopefully have ical4j 4.x then).

It would be nice to have it to support more iCalendar possibilities; on the other hand, almost nobody uses multiple-time-zone-RDATEs and in comparison to other popular clients which don't even process timezones it's quite a high-level problem.