diff --git a/caldav/compatibility_hints.py b/caldav/compatibility_hints.py index 9988d0af..078951fd 100644 --- a/caldav/compatibility_hints.py +++ b/caldav/compatibility_hints.py @@ -1058,11 +1058,10 @@ class FeatureSet: "description": "Server allows two events with identical content but different UIDs to coexist in the same calendar. Some servers reject or de-duplicate such an event ('duplicates not allowed even with a different UID'), in which case this is 'unsupported' (silently dropped) or 'ungraceful' (rejected with an error). The default 'full' is the usual behaviour.", "default": {"support": "full"}, }, - ## TODO: as for now, the tests will run towards the first calendar it will find, and most of the tests will assume the calendar is empty. This is bad. "test-calendar": { "type": "tests-behaviour", - "description": "if the server does not allow creating new calendars, then use the calendar with the given name for running tests (NOT SUPPORTED YET!), wipe the calendar between each test run (alternative for calendars not supporting the creation of new calendars is a very expensive delete objects one-by-one by uid)", - "extra_keys": { "name": "calendar name", "cleanup-regime": "thorough|pre|post|light|wipe-calendar" } + "description": "if the server does not allow creating new calendars, then use the calendar with the given name, cal_id or cal_url for running tests, wipe the calendar between each test run (alternative for calendars not supporting the creation of new calendars is a very expensive delete objects one-by-one by uid). Without it, only a calendar with 'pythoncaldav-test' in its display name or at the test fixture's cal_id is used, and the tests needing a calendar are skipped if there is none", + "extra_keys": { "name": "calendar name", "cal_id": "calendar id", "cal_url": "calendar URL", "cleanup-regime": "thorough|pre|post|light|wipe-calendar" } }, "test-calendar.compatibility-tests": { "type": "tests-behaviour", diff --git a/changes/+no-borrowed-calendar-wipe.tests.md b/changes/+no-borrowed-calendar-wipe.tests.md new file mode 100644 index 00000000..a15ab9b7 --- /dev/null +++ b/changes/+no-borrowed-calendar-wipe.tests.md @@ -0,0 +1 @@ +The integration tests no longer wipe the first calendar they find on a server where they cannot create one (Posteo, GMX and other servers where `create-calendar` is unsupported or fragile, or where MKCALENDAR is refused and the caller gives no cal_id). They use a calendar with `pythoncaldav-test` in its display name, the one at the test fixture's cal_id, or the one named under `test-calendar` in the server's features, and skip otherwise. On such servers, set a calendar aside for the tests to run them again. diff --git a/docs/design/TODO.md b/docs/design/TODO.md index 2d02dc4b..ba5b508c 100644 --- a/docs/design/TODO.md +++ b/docs/design/TODO.md @@ -8,10 +8,6 @@ `Todo._next()` in `caldav/calendarobjectresource.py` raises `TypeError` (naive vs aware) for a task with `DTSTART;VALUE=DATE` and `BY*` parts in its `RRULE`: dateutil turns the date into a naive datetime and compares it with the UTC completion timestamp, so `complete(handle_rrule=True)` fails for such tasks. Fix: convert a date DTSTART to an aware datetime (or the timestamp to a date) before both the comparison and `rrulestr()`, with a unit test. Found in the review of the fix for https://github.com/python-caldav/caldav/issues/782. -## Calendar cleanup in integration tests - -For servers where calendars cannot be created, the test will take the first available calendar it finds and wipe it. That is not acceptable without the user explicitly configuring that this is OK. - ## Async scheduling fixture leaks clients when setup fails `scheduling_setup` in `tests/test_async_integration.py` closes a client only on its `continue` branches. If `check_scheduling_support()`, `principal()`, `aget_or_create_test_calendar()` or `cleanup_calendar_objects()` raises, the fixture exits before `yield`, and that client plus every one already set up stays open. Their connections outlive the test's event loop and surface later as `ResourceWarning: loop is closed` in an unrelated test. Fix: an `AsyncExitStack` around the setup and the `yield`. diff --git a/tests/fixture_helpers.py b/tests/fixture_helpers.py index 309f52d3..d4ebcf37 100644 --- a/tests/fixture_helpers.py +++ b/tests/fixture_helpers.py @@ -7,7 +7,9 @@ import asyncio import inspect +import logging from typing import Any +from urllib.parse import unquote, urlparse async def _maybe_await(result: Any) -> Any: @@ -86,20 +88,37 @@ async def _filter_calendars_by_component_set( return matching_calendars or None -async def _find_test_calendar(calendars: list[Any]) -> Any: - """Find a dedicated test calendar by display name, or return first calendar. +## Skip reason when _find_test_calendar() finds nothing set aside for the tests. +NO_TEST_CALENDAR = ( + "no calendar could be created and none is set aside for the tests - give a " + "calendar the tests may wipe a display name containing 'pythoncaldav-test' " + "or the fixture's cal_id, or name it under 'test-calendar' in the server's features" +) + + +async def _find_test_calendar(calendars: list[Any], cal_id: str | None = None) -> Any: + """Find the calendar set aside for the tests, or None. + + A calendar is set aside when its URL path ends in ``cal_id`` or its display + name contains ``pythoncaldav-test``. Any other calendar is the user's own: the + tests wipe the calendar they use, so it must never be picked just because + the server lists it first. A different calendar can be named in the + ``test-calendar`` server config. Works with both sync and async calendar objects via _maybe_await. """ for c in calendars: + last_segment = urlparse(str(c.url)).path.rstrip("/").rsplit("/", 1)[-1] + if cal_id and unquote(last_segment) == cal_id: + return c try: - props = await _maybe_await(c.get_properties([])) - display_name = props.get("{DAV:}displayname", "") - if "pythoncaldav-test" in str(display_name): - return c - except Exception: - pass - return calendars[0] if calendars else None + display_name = await _maybe_await(c.get_display_name()) + except Exception as e: + logging.debug(f"no display name for {c.url}: {e}") + continue + if "pythoncaldav-test" in str(display_name): + return c + return None async def _get_or_create_impl( @@ -170,7 +189,7 @@ async def _get_or_create_impl( return None, False calendars = filtered - calendar = await _find_test_calendar(calendars) + calendar = await _find_test_calendar(calendars, cal_id) return calendar, created diff --git a/tests/test_async_integration.py b/tests/test_async_integration.py index 9a48cdfe..e683120e 100644 --- a/tests/test_async_integration.py +++ b/tests/test_async_integration.py @@ -265,6 +265,7 @@ async def _afixture_calendar( lives in exactly one place - see fixture_helpers.afix_calendar. """ from .fixture_helpers import ( + NO_TEST_CALENDAR, afix_calendar, arelease_calendar, atry_principal, @@ -284,7 +285,7 @@ async def _afixture_calendar( supported_calendar_component_set=supported_calendar_component_set, ) if calendar is None: - pytest.skip(f"Could not create or find {what}") + pytest.skip(f"Could not create or find {what}: {NO_TEST_CALENDAR}") yield calendar @@ -2342,7 +2343,12 @@ async def test_utf8_event(self, async_client: Any) -> None: self.skip_unless_support("save-load.event") self.skip_unless_support("create-calendar") - from .fixture_helpers import afix_calendar, arelease_calendar, atry_principal + from .fixture_helpers import ( + NO_TEST_CALENDAR, + afix_calendar, + arelease_calendar, + atry_principal, + ) principal = await atry_principal(async_client) if principal is None: @@ -2362,7 +2368,7 @@ async def test_utf8_event(self, async_client: Any) -> None: keep_name=True, ) if c is None: - pytest.skip("Could not create or find a calendar for testing") + pytest.skip(NO_TEST_CALENDAR) try: await c.add_event( near_now_ics(ev1_static).replace("Bastille Day Party", "Bringebærsyltetøyfestival") diff --git a/tests/test_caldav.py b/tests/test_caldav.py index 788b719d..a977c6d9 100644 --- a/tests/test_caldav.py +++ b/tests/test_caldav.py @@ -1577,12 +1577,17 @@ def _fixCalendar_(self, **kwargs): """ Should ideally return a new calendar, if that's not possible it should see if there exists a test calendar, if that's not - possible, give up and return the primary calendar. + possible, skip the test. The user's other calendars are never + used, as the tests wipe the calendar they get. Delegates core create-or-find logic to fixture_helpers.get_or_create_test_calendar, handling test-infrastructure concerns (caching, cleanup, cal_id defaults) here. """ - from .fixture_helpers import component_set_unobtainable, get_or_create_test_calendar + from .fixture_helpers import ( + NO_TEST_CALENDAR, + component_set_unobtainable, + get_or_create_test_calendar, + ) reason = component_set_unobtainable( self.caldav, kwargs.get("supported_calendar_component_set") @@ -1593,9 +1598,11 @@ def _fixCalendar_(self, **kwargs): if not self.is_supported("create-calendar"): if not self._default_calendar: self._default_calendar, was_created = get_or_create_test_calendar( - self.caldav, self.principal + self.caldav, self.principal, cal_id=self.testcal_id ) self._track_calendar(self._default_calendar, was_created) + if self._default_calendar is None: + pytest.skip(NO_TEST_CALENDAR) return self._default_calendar # Pre-processing: set up defaults for name and cal_id @@ -1647,6 +1654,8 @@ def _fixCalendar_(self, **kwargs): ) self._track_calendar(ret, was_created) + if ret is None: + pytest.skip(NO_TEST_CALENDAR) return ret def testCheckCompatibility(self, request) -> None: diff --git a/tests/test_fixture_helpers.py b/tests/test_fixture_helpers.py index 24b531b2..838bf2d7 100644 --- a/tests/test_fixture_helpers.py +++ b/tests/test_fixture_helpers.py @@ -41,14 +41,19 @@ def __init__( url: str = "http://dav.example.com/cal/", n_objects: int = 0, exists: bool = True, + display_name: str = "", ) -> None: self.url = url + self.display_name = display_name self.objects: list[FakeObject] = [FakeObject(self) for _ in range(n_objects)] self.deleted = False ## A Calendar object built from a cal_id is just a URL - the collection ## behind it may not exist at all, and then every request 404s. self.exists = exists + async def get_display_name(self) -> str | None: + return self.display_name + async def search(self) -> list[FakeObject]: if not self.exists: raise error.NotFoundError(f"no collection at {self.url}") @@ -251,6 +256,42 @@ def test_unicode_fixture_ids_are_cleaned_by_known_calendar_cleanup() -> None: assert fixture.testcal_id + "-unicode" in cleaned_ids +class NoCreateCalendarProbe(FunctionalFixtureProbe): + """A probe for a server that cannot create calendars (Posteo).""" + + def __init__(self) -> None: + super().__init__() + self.caldav = FakeClient({"create-calendar": False}) + + def is_supported(self, feature: str, return_type: type = bool, **kw: Any) -> Any: + if feature == "create-calendar" and return_type is bool: + return False + return super().is_supported(feature, return_type, **kw) + + +def test_sync_fixture_skips_rather_than_borrowing_a_users_calendar() -> None: + """``_fixCalendar_`` must skip, not hand out None or the user's calendar.""" + fixture = NoCreateCalendarProbe() + fixture.principal.calendars["personal"] = FunctionalCalendar( + fixture.principal, "Personal", "personal" + ) + + with pytest.raises(pytest.skip.Exception, match="test-calendar"): + fixture._fixCalendar() + + +def test_sync_fixture_borrows_the_calendar_at_its_cal_id() -> None: + """Without create-calendar the sync fixture also finds a calendar by its cal_id.""" + fixture = NoCreateCalendarProbe() + fixture.principal.calendars["personal"] = FunctionalCalendar( + fixture.principal, "Personal", "personal" + ) + at_cal_id = FunctionalCalendar(fixture.principal, None, fixture.testcal_id) + fixture.principal.calendars[fixture.testcal_id] = at_cal_id + + assert fixture._fixCalendar() is at_cal_id + + @pytest.mark.asyncio async def test_afix_calendar_creates_and_names() -> None: """Happy path: fresh calendar, display name set, deleted again on release.""" @@ -375,6 +416,75 @@ async def test_afix_calendar_returns_none_when_nothing_can_be_had() -> None: await arelease_calendar(client, calendar, created) +@pytest.mark.asyncio +async def test_a_calendar_of_the_user_is_never_borrowed() -> None: + """Without create-calendar, only a calendar set aside for the tests may be used. + + The tests wipe the calendar they get. Falling back to whatever calendar + the server lists first would wipe the user's own data - on Posteo, which + cannot create calendars, that is the account's default calendar. + """ + users_own = FakeCalendar(url="http://dav.example.com/personal/", n_objects=5) + client = FakeClient({"create-calendar": False}) + principal = FakePrincipal({"personal": users_own}) + + calendar, created = await afix_calendar(client, principal, cal_id="testcal") + + assert calendar is None + assert not created + assert len(users_own.objects) == 5, "the user's calendar must not be wiped" + + +@pytest.mark.asyncio +async def test_a_calendar_failing_mkcalendar_does_not_borrow_another() -> None: + """MKCALENDAR refused on a server claiming support: still no first-calendar fallback.""" + users_own = FakeCalendar(url="http://dav.example.com/personal/", n_objects=1) + principal = FakePrincipal({"personal": users_own}) + + async def make_calendar(**kwargs: Any) -> FakeCalendar: + raise error.AuthorizationError("Forbidden") + + principal.make_calendar = make_calendar # type: ignore[method-assign] + + calendar, created = await _get_or_create_impl(FakeClient(), principal) + + assert calendar is None + assert not created + + +@pytest.mark.asyncio +async def test_a_calendar_named_for_the_tests_is_borrowed() -> None: + """A display name containing ``pythoncaldav-test`` is the user's opt-in.""" + users_own = FakeCalendar(url="http://dav.example.com/personal/") + set_aside = FakeCalendar(url="http://dav.example.com/other/", display_name="pythoncaldav-test") + client = FakeClient({"create-calendar": False}) + principal = FakePrincipal({"personal": users_own, "other": set_aside}) + + calendar, created = await _get_or_create_impl(client, principal) + + assert calendar is set_aside + assert not created + + +@pytest.mark.parametrize( + "url", + ["http://dav.example.com/p%C3%B8-cal/", "http://dav.example.com/p%C3%B8-cal"], + ids=["trailing-slash", "no-trailing-slash"], +) +@pytest.mark.asyncio +async def test_a_calendar_at_the_cal_id_is_borrowed(url: str) -> None: + """The fixture's own cal_id is matched against the URL-quoted last path segment.""" + users_own = FakeCalendar(url="http://dav.example.com/personal/") + at_cal_id = FakeCalendar(url=url) + client = FakeClient({"create-calendar": False}) + principal = FakePrincipal({"personal": users_own, "pø-cal": at_cal_id}) + + calendar, created = await _get_or_create_impl(client, principal, cal_id="pø-cal") + + assert calendar is at_cal_id + assert not created + + @pytest.mark.asyncio async def test_configured_test_calendar_is_looked_up_and_awaited() -> None: """A ``test-calendar`` name/cal_id in the server config short-circuits creation.