Repository navigation
Fix the flaky device-list test, and move the unit tests under tests/ - #22
Merged
Merged
Conversation
DeviceList_SetProperty_UpdatesDataSource failed on the release runner with "BindingSource cannot be its own data source", having passed everywhere else, including on the runner for v1.0.0 with the same code. Building a GeneratorForm points the device search at it and queues a search on the thread pool. The search reports back by assigning the device list to whichever form is the parent when it finishes, and the parent is static, so a search started by one test reports into a later test's form. It guards that assignment with InvokeRequired, which is the right guard for a window: a form that was never shown has no handle, and InvokeRequired on a handleless control is false rather than true, so the report is made from the pool thread rather than marshalled onto the one that owns the control. Two threads then write the binding source at once. Thirty-five tests in this file build a form and none of them let go of it. DeviceUpdateThreadTests already clears the parent for exactly this reason and says so; this file now does the same. With no parent, a search that outlives the test that started it finds nothing to report to. Locally the search finishes between tests and nothing collides. The runner is slower at WMI, which is why it showed there and why it comes and goes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w
The manual suites went into tests/manual when they were added, which left the unit tests sitting beside the tests directory rather than in it. Both kinds are tests, so both live under tests now. The project moved with git mv, so the history of all nineteen files follows it. What had to change with it: every relative path in the test project goes up one more level, which is fifteen package paths and the reference to the application; the solution entry; and the path the release workflow runs vstest against, which would otherwise have looked for the assembly where it no longer is. The namespace is untouched. It was RandomNumberGenerator.Test before the move and the guidelines call for that regardless of where the folder sits, so the two mentions of that name in the guidelines are about the namespace and are left alone. Verified by rebuilding both configurations from clean and running the whole suite against each - 243 tests, and the manual chart suite against the built application, so the post build copy of the native DLL still lands where the tests look for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w
There was a problem hiding this comment.
🟢 Approval recommended
The functional change addresses the described test flakiness and the path updates are consistent; remaining findings are minor hygiene items.
Pull request overview
This PR resolves a release-blocking flake in the device-list unit test by eliminating cross-test interference from DeviceUpdateThread.Parent, and completes the requested repo tidy-up by relocating the unit test project under tests/ while updating all affected paths and documentation.
Changes:
- Fixes flakiness in
DeviceList_SetProperty_UpdatesDataSourceby clearingDeviceUpdateThread.ParentinGeneratorFormtest cleanup. - Moves the MSTest project under
tests/and updates NuGet/package paths, project references, solution entry, and release workflow vstest path. - Updates documentation to reflect the new unit-test location and distinguishes unit vs. manual test suites.
File summaries
| File | Description |
|---|---|
| tests/RandomNumberGenerator.Test/CODING_GUIDELINES_TESTS.md | Relocated unit-test coding guidelines under tests/. |
| tests/RandomNumberGenerator.Test/DeviceUpdateThread.Test.cs | Relocated device update thread unit tests under tests/. |
| tests/RandomNumberGenerator.Test/GeneratorForm.Test.cs | Clears DeviceUpdateThread.Parent during test cleanup to prevent cross-test device-thread callbacks. |
| tests/RandomNumberGenerator.Test/HistogramChart.Test.cs | Relocated histogram chart unit tests under tests/. |
| tests/RandomNumberGenerator.Test/Properties/AssemblyInfo.cs | Relocated test project assembly metadata under tests/. |
| tests/RandomNumberGenerator.Test/RNGChart.Test.cs | Relocated RNG chart unit tests under tests/. |
| tests/RandomNumberGenerator.Test/RNGDevice.Test.cs | Relocated RNG device unit tests under tests/. |
| tests/RandomNumberGenerator.Test/RNGDeviceTimer.Test.cs | Relocated RNG device timer unit tests under tests/. |
| tests/RandomNumberGenerator.Test/RNGSessionData.Test.cs | Relocated RNG session data unit tests under tests/. |
| tests/RandomNumberGenerator.Test/RNGSessionDataFile.Test.cs | Relocated RNG session data file unit tests under tests/. |
| tests/RandomNumberGenerator.Test/RNGSessionTimer.Test.cs | Relocated RNG session timer unit tests under tests/. |
| tests/RandomNumberGenerator.Test/RNGXMLReader.Test.cs | Relocated RNG XML reader unit tests under tests/. |
| tests/RandomNumberGenerator.Test/RNGXMLWriter.Test.cs | Relocated RNG XML writer unit tests under tests/. |
| tests/RandomNumberGenerator.Test/RandomNumberGenerator.Test.csproj | Updates package HintPaths/imports and project reference for the new tests/ location. |
| tests/RandomNumberGenerator.Test/RandomNumberGenerator.sln | Added a solution file under the test directory (appears to be a placeholder). |
| tests/RandomNumberGenerator.Test/SignificanceTest.Test.cs | Relocated significance test unit tests under tests/. |
| tests/RandomNumberGenerator.Test/StatisticalAnalysis.Test.cs | Relocated statistical analysis unit tests under tests/. |
| tests/RandomNumberGenerator.Test/TargetValues.Test.cs | Relocated target values unit tests under tests/. |
| tests/RandomNumberGenerator.Test/XMLDataPoint.Test.cs | Relocated XML data point unit tests under tests/. |
| tests/RandomNumberGenerator.Test/packages.config | Relocated NuGet package list under tests/. |
| tests/manual/README.md | Updates manual test docs to reference the new unit-test path. |
| README.md | Updates test invocation paths and documents tests/manual/ alongside unit tests. |
| RandomNumberGenerator.sln | Updates the test project path to tests\\RandomNumberGenerator.Test\\.... |
| CLAUDE.md | Updates project layout and build/test commands for the new unit-test location. |
| .github/workflows/release.yml | Updates vstest path in the release workflow to the new test output location. |
Review details
- Files reviewed: 8/25 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -0,0 +1 @@ | |||
| No newline at end of file | |||
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
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.
Two commits. The first unblocks releases; the second is the tidy-up you asked for.
1. The flaky test that failed the v1.0.1 release
DeviceList_SetProperty_UpdatesDataSourcefailed on the release runner with "BindingSource cannot be its own data source", having passed locally and on the runner for v1.0.0 with identical code — the only change between those two commits was the workflow file.Root cause: building a
GeneratorFormpoints the device search at it and queues a search on the thread pool. The parent isstatic, so a search started by one test reports into a later test's form. The report is guarded byInvokeRequired, which is the right guard for a window — but a form that was never shown has no handle, andInvokeRequiredon a handleless control returns false rather than true. So the report is made from the pool thread instead of being marshalled, and two threads write the binding source at once.Thirty-five tests in that file build a form and none let go of it.
DeviceUpdateThreadTestsalready clears the parent in its cleanup "to avoid test interdependencies";GeneratorFormTestsnow does the same. With no parent, a search that outlives the test that started it finds nothing to report to.This is why it comes and goes: locally the WMI search finishes between tests, and on a slower runner it does not.
A note on what this does not fix. The
elsebranch inUpdateDeviceListis still unsound in principle —InvokeRequired == falsedoes not mean "safe to touch" on a handleless control. It is unreachable in the shipped application, because the form is always shown by the time a search completes, so I have left the product alone rather than widenIGeneratorFormuninvited. Worth a decision separately.2. The unit tests move under
tests/tests/manualarrived with the manual suites, which left the unit tests sitting beside the tests directory rather than in it.Moved with
git mv, so the history of all nineteen files follows. What had to move with it:CLAUDE.md,README.mdandtests/manual/README.mdThe namespace is untouched — it was
RandomNumberGenerator.Testbefore and the guidelines ask for that whatever the folder is called, so the two mentions of that name in the guidelines are about the namespace and were left alone.Verification
Clean rebuild of both configurations, full suite against each: 243/243. Plus a manual suite against the built application, which confirms the post-build copy of the native DLL still lands where the tests look for it after the move.
🤖 Generated with Claude Code
https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w