|
| 1 | +# Code Review — 2026-03-13 |
| 2 | + |
| 3 | +Covers: `src/caldav_server_tester/checks_base.py`, `checker.py`, |
| 4 | +`caldav_server_tester.py`, `checks.py`. |
| 5 | + |
| 6 | +--- |
| 7 | + |
| 8 | +## Critical bugs |
| 9 | + |
| 10 | +### 1. Dead / broken assertion logic in `Check.run_check()` (`checks_base.py:103-125`) |
| 11 | + |
| 12 | +Two problems in the block that is supposed to verify every declared feature |
| 13 | +was actually checked: |
| 14 | + |
| 15 | +**a) `missing_keys` is clobbered on the very next line after it is computed:** |
| 16 | + |
| 17 | +```python |
| 18 | +missing_keys = self.features_to_be_checked - new_keys # line 103 |
| 19 | +parent_keys = () # line 104 |
| 20 | +... |
| 21 | +missing_keys = set() # line 108 ← overwrites! |
| 22 | +for missing in missing_keys: # iterates empty set → no-op |
| 23 | + ... |
| 24 | +assert not missing_keys # always passes trivially |
| 25 | +``` |
| 26 | + |
| 27 | +The intent was to remove from `missing_keys` any key whose parent prefix |
| 28 | +already appears in `keys_after`. As written this is completely dead — missing |
| 29 | +features are never detected. |
| 30 | + |
| 31 | +**b) `parent_keys` is a `tuple` but `.add()` is called on it (line 115):** |
| 32 | + |
| 33 | +```python |
| 34 | +parent_keys = () # tuple |
| 35 | +... |
| 36 | +parent_keys.add(feature_) # AttributeError: 'tuple' object has no attribute 'add' |
| 37 | +``` |
| 38 | + |
| 39 | +This code is currently unreachable (because `missing_keys` is always empty), |
| 40 | +but the latent bug will fire the moment the `missing_keys = set()` on line 108 |
| 41 | +is removed. |
| 42 | + |
| 43 | +**c) `extra_keys` mutation-while-iterating is latent, not currently firing (lines 120-125):** |
| 44 | + |
| 45 | +```python |
| 46 | +extra_keys = new_keys - self.features_to_be_checked |
| 47 | +for x in extra_keys: |
| 48 | + for y in parent_keys: # parent_keys is always () — inner body never runs |
| 49 | + if x.startswith(y): |
| 50 | + extra_keys.remove(x) # would be RuntimeError if reached |
| 51 | +``` |
| 52 | + |
| 53 | +Because `parent_keys` is always an empty tuple (the `missing_keys` loop |
| 54 | +above never runs to populate it), the inner `for y` body is never entered |
| 55 | +and `extra_keys.remove(x)` is never reached. The bug is real but currently |
| 56 | +harmless. If the `missing_keys = set()` clobber (issue 1a) is ever fixed, |
| 57 | +this will start throwing `RuntimeError` for non-empty `extra_keys`. |
| 58 | + |
| 59 | +--- |
| 60 | + |
| 61 | +### 2. Wrong variable in `CheckRecurrenceSearch._run_check()` (`checks.py:1207`) |
| 62 | + |
| 63 | +```python |
| 64 | +far_future_recurrence = cal.search( |
| 65 | + start=datetime(2045, 3, 12, tzinfo=utc), ... # line 1201 |
| 66 | +) |
| 67 | +self.set_feature( |
| 68 | + "search.recurrences.includes-implicit.infinite-scope", |
| 69 | + len(events) == 1 # line 1207 — uses `events`, not `far_future_recurrence`! |
| 70 | +) |
| 71 | +``` |
| 72 | + |
| 73 | +`events` at this point holds the February 2000 search result (set at line |
| 74 | +1144), which is the same value tested by `implicit_datetime`. The |
| 75 | +`far_future_recurrence` variable that was just computed is never used. The |
| 76 | +feature effectively measures the same thing as |
| 77 | +`search.recurrences.includes-implicit.event`, which makes it useless. |
| 78 | + |
| 79 | +Should be `len(far_future_recurrence) == 1`. |
| 80 | + |
| 81 | +--- |
| 82 | + |
| 83 | +## Significant issues |
| 84 | + |
| 85 | +### 3. Global monkey-patch of `Calendar.search` in `ServerQuirkChecker.__init__()` (`checker.py:36-43`) |
| 86 | + |
| 87 | +```python |
| 88 | +if not hasattr(Calendar, "_original_search"): |
| 89 | + Calendar._original_search = Calendar.search |
| 90 | + def delayed_search(self, *args, **kwargs): |
| 91 | + time.sleep(delay) |
| 92 | + return Calendar._original_search(self, *args, **kwargs) |
| 93 | + Calendar.search = delayed_search |
| 94 | +``` |
| 95 | + |
| 96 | +- This patches the *class*, not an instance, so the patch affects every |
| 97 | + `Calendar` in the process — including code paths unrelated to this tester. |
| 98 | +- The `delay` value is captured by closure from whichever |
| 99 | + `ServerQuirkChecker` was constructed first. A second instance with a |
| 100 | + different delay will silently use the first one's value. |
| 101 | +- The patch persists for the lifetime of the process even after the |
| 102 | + `ServerQuirkChecker` is destroyed. |
| 103 | + |
| 104 | +A safer approach: wrap at the instance level or pass the delay through |
| 105 | +another mechanism. |
| 106 | + |
| 107 | +### 4. `missing_keys` / `features_to_be_checked` invariant is never enforced |
| 108 | + |
| 109 | +As shown in issue #1, the assertion that every declared feature was checked |
| 110 | +is a no-op. New check classes can silently omit setting features they |
| 111 | +declared in `features_to_be_checked` and the runtime will not notice. |
| 112 | + |
| 113 | +### 5. Hardcoded UID list in `cleanup()` is out of sync with `PrepareCalendar` (`checker.py:86-102`) |
| 114 | + |
| 115 | +The fallback cleanup path lists UIDs by hand. `PrepareCalendar` adds |
| 116 | +additional objects (e.g. `csc_event_with_alarm`, `csc_yearly_recurring_allday_event`, |
| 117 | +`weeklymeeting`, `csc_url_check`, `csc_no_time_range_*`, sync-test events, |
| 118 | +timezone-test events) that are *not* in this list. If calendar deletion is |
| 119 | +not supported, those objects will be left behind. |
| 120 | + |
| 121 | +To avoid this in the future, we should: |
| 122 | + |
| 123 | +1) have code comments in the prepare-calendar warning that anything added needs to be cleaned up |
| 124 | +2) Perhaps have an option for searching for objects with uid starting with `csc_` after cleanup |
| 125 | + |
| 126 | +--- |
| 127 | + |
| 128 | +## Minor bugs / typos |
| 129 | + |
| 130 | +### 6. Typo in `checker.py:200` |
| 131 | + |
| 132 | +```python |
| 133 | +lines.append(f"Fature support level found: {support}") |
| 134 | +``` |
| 135 | + |
| 136 | +"Fature" → "Feature". |
| 137 | + |
| 138 | +### 7. `_compute_diff()` called twice when `return_what=str` and `show_diff=True` (`checker.py:156-157, 209-212`) |
| 139 | + |
| 140 | +When formatting as plain text the method builds a `ret` dict (including |
| 141 | +`ret["diff"]` at line 157) that is never used for the text path. Then |
| 142 | +`_compute_diff()` is called again at line 209. Either remove the |
| 143 | +`ret["diff"]` assignment for the `str` path or reuse the already-computed |
| 144 | +value. |
| 145 | + |
| 146 | +### 8. Redundant `import logging` inside `CheckMakeDeleteCalendar._try_make_calendar()` (`checks.py:110`) |
| 147 | + |
| 148 | +`logging` is already imported at the top of the module (line 1). |
| 149 | + |
| 150 | +### 9. Double assignment of `features_to_be_checked` in `PrepareCalendar` (`checks.py:256-258`) |
| 151 | + |
| 152 | +```python |
| 153 | +features_to_be_checked = set() # line 256 — immediately overwritten |
| 154 | +depends_on = {CheckMakeDeleteCalendar} |
| 155 | +features_to_be_checked = { # line 258 — the real value |
| 156 | + "save-load.event.recurrences", |
| 157 | + ... |
| 158 | +} |
| 159 | +``` |
| 160 | + |
| 161 | +The first assignment is dead code. |
| 162 | + |
| 163 | +### 10. Missing `set_feature` for `search.is-not-defined.class` in `CheckIsNotDefined` (`checks.py:984-1001`) |
| 164 | + |
| 165 | +`class_works` is computed and used in the summary `results` dict (line |
| 166 | +1033) that drives the overall `search.is-not-defined` feature value, but |
| 167 | +there is no corresponding `self.set_feature("search.is-not-defined.class", |
| 168 | +...)` call — unlike the analogous `.category` and `.dtend` sub-features. |
| 169 | +This means the class sub-feature is not individually observable. |
| 170 | + |
| 171 | +--- |
| 172 | + |
| 173 | +## Style / quality concerns |
| 174 | + |
| 175 | +### 11. Pervasive bare `except:` clauses |
| 176 | + |
| 177 | +Many places catch all exceptions including `SystemExit` and |
| 178 | +`KeyboardInterrupt`, e.g. `checks.py:61, 93, 117, 124, 211, 287, 374, 410` |
| 179 | +and others. These should be at minimum `except Exception:`. |
| 180 | + |
| 181 | +### 12. `type(foo) == date` instead of `isinstance` (`checks.py:30`) |
| 182 | + |
| 183 | +```python |
| 184 | +asdate = lambda foo: foo if type(foo) == date else foo.date() |
| 185 | +``` |
| 186 | + |
| 187 | +- `type(x) == date` is False for `datetime` (a subclass of `date`), which is |
| 188 | + arguably intentional here, but it should be commented if so. |
| 189 | +- Assigning a `lambda` to a name is a PEP 8 anti-pattern; use `def`. |
| 190 | + |
| 191 | +### 13. Misleading TODO comment in `caldav_server_tester.py:108-110` |
| 192 | + |
| 193 | +```python |
| 194 | +## TODO: this looks somewhat convoluted ... wouldn't it be better with this: |
| 195 | +#return_what = output_format if output_format in ("json", "yaml", "hints" else str |
| 196 | +``` |
| 197 | + |
| 198 | +The commented-out line has a syntax error (missing closing paren). If the |
| 199 | +intent is to show the improved form, fix the syntax; if the idea was |
| 200 | +abandoned, remove the comment. |
| 201 | + |
| 202 | +### 14. `assert` used for runtime validation in non-test code |
| 203 | + |
| 204 | +`checks_base.py` uses `assert` to validate runtime invariants (lines 64, |
| 205 | +117, 125) and `checks.py` uses them in logic paths (e.g. line 1143). These |
| 206 | +are silently skipped when Python runs with `-O`. Replace with explicit |
| 207 | +`if ... raise` checks where the invariant matters. |
| 208 | + |
| 209 | +### 15. `PrepareCalendar._run_check()` is ~415 lines long (`checks.py:272-687`) |
| 210 | + |
| 211 | +It creates all test fixtures, handles fallbacks for tasks and journals, |
| 212 | +manages three separate calendar handles, and sets a dozen features. Consider |
| 213 | +splitting into focused helpers (e.g. `_prepare_task_calendar`, |
| 214 | +`_prepare_journal_calendar`, `_create_test_events`). |
| 215 | + |
| 216 | +--- |
| 217 | + |
| 218 | +## Summary table |
| 219 | + |
| 220 | +| # | Severity | File | Location | Issue | |
| 221 | +|---|----------|------|----------|-------| |
| 222 | +| 1a | Critical | `checks_base.py` | 103–117 | `missing_keys` overwritten; assertion always passes | |
| 223 | +| 1b | Critical | `checks_base.py` | 115 | `tuple.add()` — latent `AttributeError` | |
| 224 | +| 1c | Medium | `checks_base.py` | 120–125 | Latent `RuntimeError` (set modified during iteration) — currently dead because `parent_keys` is always empty | |
| 225 | +| 2 | Critical | `checks.py` | 1207 | `infinite-scope` uses wrong variable (`events` vs `far_future_recurrence`) | |
| 226 | +| 3 | High | `checker.py` | 36–43 | Global monkey-patch of `Calendar.search` leaks across process | |
| 227 | +| 4 | High | `checks_base.py` | 100–125 | Feature-check invariant never enforced | |
| 228 | +| 5 | High | `checker.py` | 86–102 | Cleanup UID list incomplete vs objects `PrepareCalendar` creates | |
| 229 | +| 6 | Low | `checker.py` | 200 | Typo: "Fature" | |
| 230 | +| 7 | Low | `checker.py` | 156–157, 209 | `_compute_diff()` called twice for text format | |
| 231 | +| 8 | Low | `checks.py` | 110 | Redundant `import logging` inside method | |
| 232 | +| 9 | Low | `checks.py` | 256 | Dead first assignment of `features_to_be_checked` | |
| 233 | +| 10 | Medium | `checks.py` | 984–1001 | Missing `set_feature` for `search.is-not-defined.class` sub-feature | |
| 234 | +| 11 | Medium | `checks.py` | many | Bare `except:` should be `except Exception:` | |
| 235 | +| 12 | Low | `checks.py` | 30 | `type(x) == date` and `lambda` assigned to name | |
| 236 | +| 13 | Low | `caldav_server_tester.py` | 108–110 | Misleading TODO with syntax-error example | |
| 237 | +| 14 | Medium | `checks_base.py`, `checks.py` | various | `assert` used for production runtime checks | |
| 238 | +| 15 | Low | `checks.py` | 272–687 | `PrepareCalendar._run_check` too long, needs decomposition | |
0 commit comments