Commit 59616e3
test: make FileDataSource auto-update tests more robust against timing issues (#192)
**Requirements**
- [x] I have added test coverage for new or changed functionality
- [x] I have followed the repository's [pull request submission
guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests)
- [ ] I have validated my changes against all supported platform
versions
**Related issues**
Fixes flaky test
`ModifiedFileIsReloadedEvenIfOneFileIsMissingIfSkipMissingPathsIsSet`
observed in CI: [test run
logs](https://productionresultssa1.blob.core.windows.net/actions-results/13f4e4cd-7974-47b0-b829-de269e609d1f/workflow-job-run-843a6286-f650-51c4-b0dc-5c1e9e4974ff/logs/job/job-logs.txt)
**Describe the solution you've provided**
Added a new `AssertHelpers.ExpectJsonValue<T>()` helper that loops on
`ExpectValue()` until the received value's JSON matches the expected
JSON (or times out). This preserves the original `AssertJsonEqual`
assertions while handling race conditions from file watcher events.
The helper uses `JsonTestValue` parameters to match the return type of
`DataSetAsJson()` and the expected signature of `AssertJsonEqual()`.
Tests modified in `FileDataSourceTest.cs`:
- `ModifiedFileIsReloadedEvenIfOneFileIsMissingIfSkipMissingPathsIsSet`
- `IfFlagsAreBadAtStartTimeAutoUpdateCanStillLoadGoodDataLater`
Also fixed a separate flaky test in `PollingDataSourceTest.cs`:
-
`SuccessfulRequestCausesDataToBeStoredAndDataSourceInitializedMetadata`
- added `initTask.Wait()` before checking `Initialized` flag to avoid
race condition
**Describe alternatives you've considered**
- Adding `Thread.Sleep()` delays - rejected as this would make tests
slower and still potentially flaky
- Using `ExpectPredicate()` with custom predicates - rejected per review
feedback to preserve original `AssertJsonEqual` assertions for clarity
- Modifying the application code - rejected per guidelines to focus on
fixing test code rather than application code
**Additional context**
- Link to Devin run:
https://app.devin.ai/sessions/151f389666f447c78d9990e42bc57b48
- Requested by: [email protected]
**Human review checklist**
- [ ] Verify `ExpectJsonValue` correctly handles the timeout and doesn't
silently swallow real failures (it catches `XunitException` to skip
intermediate events)
- [ ] Confirm the 30-second timeout is appropriate (matches existing
patterns in the codebase)
- [ ] Verify the original assertions are preserved and we're testing the
same behavior
- [ ] Note: The `AssertJsonEqual` call after `ExpectJsonValue` is
intentionally redundant for readability per review feedback
---------
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: [email protected] <[email protected]>1 parent a50515e commit 59616e3
File tree
3 files changed
+58
-6
lines changed- pkgs/sdk/server/test
- Internal/DataSources
3 files changed
+58
-6
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
81 | 116 | | |
82 | 117 | | |
Lines changed: 18 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
270 | 270 | | |
271 | 271 | | |
272 | 272 | | |
273 | | - | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
274 | 281 | | |
275 | 282 | | |
276 | 283 | | |
| |||
291 | 298 | | |
292 | 299 | | |
293 | 300 | | |
294 | | - | |
295 | | - | |
296 | | - | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
297 | 304 | | |
298 | 305 | | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
299 | 313 | | |
300 | 314 | | |
301 | 315 | | |
| |||
Lines changed: 5 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
96 | 96 | | |
97 | 97 | | |
98 | 98 | | |
99 | | - | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
100 | 103 | | |
101 | | - | |
| 104 | + | |
102 | 105 | | |
103 | 106 | | |
104 | 107 | | |
| |||
0 commit comments