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 #92

Closed ArnyminerZ closed 1 year ago

ArnyminerZ commented 1 year ago

@rfc2822 Please take a look at the updated tests also. I've changed some dates that seemed wrong to me.

ArnyminerZ commented 1 year ago

Oops, some failing tests

ArnyminerZ commented 1 year ago

So now it seems that we are having the same issue as in #49.

We are instantiating all the dates in testBuildEvent_AllDay_DtEnd_NoDuration_Recurring without timezone, so it takes the default one (local). The behavior of the current implementation of androidStringToRecurrenceSet is to not add the timezone prefix ([TIMEZONE];) if the timezone is UTC, but add it otherwise. This test, as an example, tests for 20200601T000000Z,20210601T000000Z,20220601T000000Z, without timezone, so it's failing for me with:

org.junit.ComparisonFailure: expected:<...00Z,20210601T000000Z[,]20220601T000000Z> but was:<...00Z,20210601T000000Z[
Europe/Madrid;]20220601T000000Z>

which is my timezone. I think the best approach would be to add an "ignore timezone" regex, since substringAfter doesn't apply now.

rfc2822 commented 1 year ago
org.junit.ComparisonFailure: expected:<...00Z,20210601T000000Z[,]20220601T000000Z> but was:<...00Z,20210601T000000Z[
Europe/Madrid;]20220601T000000Z>

But isn't that invalid? The tzid must be the first thing in the line, so this would be invalid in Android.

I'm not a bit confused. In my imagination, the test should define the time zone:

       val values = buildEvent(false) {
            dtStart = DtStart(Date("20200601"))
            dtEnd = DtEnd(Date("20200701"))
            rDates += RDate(DateList("20210601", Value.DATE))         // dates are always UTC
            rDates += RDate(DateList("20220601T120030", Value.DATE_TIME))     // explicitly specify time zone here
        }
        assertEquals(1, values.getAsInteger(Events.ALL_DAY))

And then the method should return two lines, one with UTC (because dates are always expressed in UTC) and one with the specified time zone.

        assertEquals("20200601T000000Z,20210601T000000Z\n" +
                        "<the tzid;>20220601T000000", values.getAsString(Events.RDATE))

Because that's what we want to achieve – one time zone ID (or only UTC, which counts as time zone UTC) per line.

ArnyminerZ commented 1 year ago

Yeah, I confused the [] in the assertion. It has now been fixed and is passing.

rfc2822 commented 1 year ago

@ArnyminerZ I have updated the tests so that they run again. In tests, assumptions should be made explicit as good as possible. For instance, tests shouldn't rely on a default time zone, so we can set the timezone explicitly when a DateTime() is created. I have specified the time zones (which were previously implicit) and changed the expected values of the asserts.

We should also use assertEquals() instead of regexp matching whenever possible. This is a result of making the inputs explicit: explicit inputs → well-defined outputs → we can check the exact expected value.

Also I have replaced {tzVienna.id} etc by Europe/Vienna. Both have advantages and disadvantages, but I like it when tests are as explicit as possible.

ArnyminerZ commented 1 year ago

Replaced by #104, since we migrated to library