test: assert timezone invariants across every TZDB identifier - #253
Merged
Conversation
Every timezone defect closed by #234, #235, #238 and #239 had the same shape: a category of DateTimeZone name nobody had thought of. Each was found by hand, one at a time, and each needed its own targeted fix and its own targeted test. Sweep every name DateTimeZone::listIdentifiers(ALL_WITH_BC) returns, plus the offset and abbreviation spellings only DateTimeZone accepts, through three event shapes and all five generators, asserting the five invariants from #252: 1. A TZID parameter carries the whole zone name, so a name holding a colon shows up as the truncation a client would read (RFC 5545 3.1). 2. Every VTIMEZONE states at least one observance (RFC 5545 3.6.5). 3. Every endpoint written with a TZID resolves, against the observances in the file, to the offset PHP reports for that zone at that instant. 4. A ctz, stz or etz is one of the link's own zones, the link reports resolvable timezones, and the name needs no URL encoding (RFC 3986). 5. No generator raises a PHP warning, notice or deprecation. Nothing is measured against a hardcoded transition date. The tzdb ships with PHP and moves, and the CI matrix spans three PHP versions carrying different releases of it, so every expectation is asked of DateTimeZone at runtime. A wider spread of tzdata widens the search. Invariants 2 and 5 also sweep the referencedTimezones() extension point. A zone with no transition table is never resolvable, so it reaches no TZID through an endpoint, and a subclass putting one in a component is the only path that gets there at all. That is the path the empty VTIMEZONE and its warning came down. Verified by mutation. Restoring the pre-#249 observance handling trips invariant 3 for 194 zones, Casablanca included; dropping the getTransitions() === false guard trips 2 and 5; widening the resolvable set trips 1 and 4. The full sweep adds ~120ms. The targeted regression tests stay as they are, since they document why each individual rule exists.
A PHP linked against the system timezone database, which is how most distributions build it and how the CI runners get theirs, enumerates the files in its zoneinfo directory rather than a list of zones. That directory holds more than zones: `leapseconds` is listed there, and DateTimeZone::__construct() then refuses it, so the sweep errored on every runner while passing against the tzdata PHP bundles on macOS. A name that cannot become a DateTimeZone cannot reach this library either, so it is out of scope rather than a defect. It is recognised by trying to construct one rather than by naming it, since another build may ship a different set of such files.
Four gaps found reviewing the sweep against the ICS work in flight. Invariant 1 expected a DTEND;TZID= to name the end zone, which holds only while a TZID is reachable exclusively through the distinct-zone path. Where the two zones collapse into one, the end is a second spelling already folded into the start's and the generators name the start's zone on both endpoints, correctly. Both invariant 1 and invariant 3 now ask one helper which zone a property names and at what instant, so the two cannot drift apart. The sweep passed no ICS options, so a recurring event was never swept. Whether an RRULE is present can decide whether the endpoints are written as local times named by a TZID at all, since a recurrence repeats the local time of its DTSTART, so that path went unasked. A shape now carries the options it needs. The flight shape always departed the swept zone for a fixed partner, so the swept zone was never an arrival. hasResolvableTimezones() judges the start and the end by different rules, so a reversed shape asks a genuinely different question. It costs one more pass, not the cross product. The note on EVENT_START reasoned only about a daylight saving overlap, which is the hazard for a 09:00 start. The all-day shape starts at midnight, where the hazard is the opposite one: a zone that springs forward at midnight has no 00:00 to name at all. Also records which direction invariant 4 guards. It reads only the parameters that were written, so it catches a name that should not have been emitted; an emission wrongly dropped leaves nothing there to judge and is caught by the snapshots and the targeted tests instead.
The shape that separates the two rules for which zone a DTEND names was missing, so the previous commit's fix passed either way and proved nothing. Every shape so far either names one zone at both ends or names two that genuinely differ; neither tells the end zone's own name apart from the zone actually written into the TZID. This one ends in a bare offset equal to the one the swept zone is on. It shares that offset and names no place, so the pair collapses into the start's zone, and the generators name the start's zone on both endpoints while the end zone still answers to `+02:00`. Reverting the rule now fails against every zone that observes a change, which is what the fix is there to prevent once a single-zone event can carry a TZID.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context and Purposes
Closes #252.
Every timezone defect closed by #234, #235, #238 and #239 had the same shape: a category of
DateTimeZonename nobody had thought of. Each was found by hand or by a reviewer, one at a time, and each needed its own targeted fix and its own targeted test. That question ("does this class of zone name break something") should be asked by a test, not by whoever happens to be reading the diff.tests/TimezoneInvariantsTest.phpsweeps every nameDateTimeZone::listIdentifiers(ALL_WITH_BC)returns (598 on PHP 8.5), plus the offset and abbreviation spellings onlyDateTimeZoneaccepts (+02:00,-05:00,Z,CEST,gmt+0,gmt-0), through three event shapes and all five generators.Invariants
TZIDparameter carries the whole zone name. A param-value ends at the first colon unless quoted, so a name holding one shows up as the truncation a client would actually readVTIMEZONEstates at least oneSTANDARDorDAYLIGHTobservanceTZIDresolves, against the observances in the file, to the offset PHP reports for that zone at that instantctz,stzoretznames one of the link's own zones, the link reportshasResolvableTimezones(), and the name needs no URL encodingEach test collects what it finds wrong across the whole sweep and asserts it found nothing, so one run names every zone that broke rather than stopping at the first.
Notes
Nothing is measured against a hardcoded transition date. The tzdb ships with PHP and moves, and the CI matrix spans three PHP versions that may carry different releases of it, so every expectation is asked of
DateTimeZoneat runtime. A wider spread of tzdata widens the search rather than breaking the suite.Invariants 2 and 5 also sweep the
referencedTimezones()extension point. A zone with no transition table is never resolvable, so it reaches noTZIDthrough an endpoint, and a subclass putting one into a component is the only path that gets there at all. That is the path the emptyVTIMEZONEand itsgetTransitions()warning came down, so without this pass those two invariants would have no teeth for exactly the zone class that produced the defect.Verified by mutation, since a sweep that asserts nothing useful passes just as quietly as one that does:
Africa/CasablancaincludedgetTransitions() === falseguardRuntime: the full sweep adds ~120ms, so the pair pass runs against a fixed partner zone for every identifier rather than being sampled. A full cross product was left out: it is ~366k links for coverage the fixed partner already gives.
The targeted regression tests added with each fix stay exactly as they are, since they document why each individual rule exists.
Deliberately out of scope
Invariant 4 proves the generators obey the library's own rule about which names are worth emitting. It does not prove the rule is right, because no offline test knows what Google actually resolves. That ground truth needs live checks against the services, which need a logged in account and cannot run in CI. The one part of invariant 4 that holds regardless of what the library decides is the encoding check, which is why it is asserted separately.