[fix] testcases for ci run 20260521#3467
Open
nandishjpatel wants to merge 6 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR consolidates test reliability improvements across six test and implementation files. The SEM simulator's acquisition duration calculation now includes settling time proportional to scan row count, with corresponding test expectations updated to use the scanner's settleTime attribute instead of hardcoded values. Multiple test files replace fixed timing delays with deadline-based polling loops: symphotime tests poll for measurement completion, and startup tests poll for window appearance. A new validator-aware text simulation helper replaces low-level keyboard simulation in UI tests, filtering input characters against control validators and dispatching proper commit events. Viewport resize assertions now use measured canvas dimensions instead of hardcoded assumptions, and unused imports are removed. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/driver/test/simsem_test.py`:
- Line 168: Replace the fragile wall-clock equality assertions that use
assertAlmostEqual(duration, expected_duration, delta=0.1) with a one-sided
lower-bound check (e.g., self.assertGreaterEqual(duration, expected_duration,
msg=...)) so tests only require the elapsed time to be at least the theoretical
duration (CI jitter tends to lengthen, not shorten, runs); update the assertion
message accordingly and apply this change for every occurrence of the pattern in
simsem_test.py (the assertAlmostEqual(...) calls at the reported locations).
In `@src/odemis/gui/test/comp_text_test.py`:
- Line 92: Add type annotations to the helper function _simulate_type: annotate
the second parameter as text: str and the first parameter as a general ctrl: Any
(or a more specific widget/control type if available), and add a return
annotation -> None; also ensure typing.Any is imported if not already present.
This updates the signature of _simulate_type to include parameter and return
type hints (ctrl, text, -> None) to satisfy the Python file guideline while
preserving existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48671e3b-6da2-4504-bf9d-418d94cd9c67
📒 Files selected for processing (6)
src/odemis/acq/move.pysrc/odemis/driver/test/simsem_test.pysrc/odemis/driver/test/symphotime_test.pysrc/odemis/gui/test/comp_text_test.pysrc/odemis/gui/test/comp_viewport_test.pysrc/odemis/odemisd/test/start_test.py
|
|
||
| self.assertEqual(im.shape, self.size[::-1]) | ||
| self.assertGreaterEqual(duration, expected_duration, "Error execution took %f s, less than exposure time %d." % (duration, expected_duration)) | ||
| self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration)) |
There was a problem hiding this comment.
Overly strict wall-clock equality will likely make these tests flaky.
At Line 168 (and the same pattern at Line 201, Line 220, Line 259, Line 273, Line 287, Line 397, Line 628), assertAlmostEqual(..., delta=0.1) assumes elapsed time stays tightly centered on theoretical duration. In CI, scheduler jitter usually makes elapsed time only reliably bounded from below.
Suggested stabilization pattern
- self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration))
+ self.assertGreaterEqual(
+ duration,
+ expected_duration,
+ "Execution took %f s, less than expected %f s." % (duration, expected_duration),
+ )
+ self.assertLess(
+ duration,
+ expected_duration + 1.0,
+ "Execution took %f s, unexpectedly slower than expected %f s." % (duration, expected_duration),
+ )Also applies to: 201-201, 220-220, 259-259, 273-273, 287-287, 397-397, 628-628
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/driver/test/simsem_test.py` at line 168, Replace the fragile
wall-clock equality assertions that use assertAlmostEqual(duration,
expected_duration, delta=0.1) with a one-sided lower-bound check (e.g.,
self.assertGreaterEqual(duration, expected_duration, msg=...)) so tests only
require the elapsed time to be at least the theoretical duration (CI jitter
tends to lengthen, not shorten, runs); update the assertion message accordingly
and apply this change for every occurrence of the pattern in simsem_test.py (the
assertAlmostEqual(...) calls at the reported locations).
There was a problem hiding this comment.
Pull request overview
This PR aims to make CI runs more reliable by reducing timing-/UI-dependent flakiness in several test suites and by ensuring posture state is updated after certain stage posture switches.
Changes:
- Replace fixed sleeps with polling/timeouts in GUI and driver tests.
- Make GUI viewport resize assertions less dependent on platform-specific window decoration behavior.
- Update posture managers to refresh the current posture after cryo sample-position switching completes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/odemisd/test/start_test.py | Replaces a fixed startup sleep with polling for the GUI window. |
| src/odemis/gui/test/comp_viewport_test.py | Adjusts resize test assertions to tolerate platform-dependent canvas sizing. |
| src/odemis/gui/test/comp_text_test.py | Replaces UIActionSimulator usage with a focus-independent typing simulation helper. |
| src/odemis/driver/test/symphotime_test.py | Replaces a fixed post-unsubscribe sleep with polling for measurement stop. |
| src/odemis/driver/test/simsem_test.py | Changes acquisition timing assertions from lower-bound checks to near-equality checks. |
| src/odemis/acq/move.py | Updates posture after cryo sample-position switch tasks complete. |
Comments suppressed due to low confidence (7)
src/odemis/driver/test/simsem_test.py:202
- Using
assertAlmostEqual(duration, expected_duration, delta=0.1)makes this test fail whenever the acquisition takes slightly longer than the expected duration (scheduler/CI overhead). This should generally be a lower-bound assertion (>= expected exposure time) to avoid flaky failures.
start = time.time()
vector_data = self.sed.data.get()
duration = time.time() - start
self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration))
src/odemis/driver/test/simsem_test.py:221
assertAlmostEqual(duration, expected_duration, delta=0.1)enforces an upper bound on acquisition time; under CI load this can easily exceedexpected_duration + 0.1even if behavior is correct. Prefer assertingduration >= expected_duration(optionally with a small tolerance) to validate it wasn’t faster than the simulated dwell.
start = time.time()
im = self.sed.data.get()
duration = time.time() - start
self.assertEqual(im.shape, self.size[::-1])
self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration))
self.assertIn(model.MD_DWELL_TIME, im.metadata)
src/odemis/driver/test/simsem_test.py:260
- Same issue:
assertAlmostEqual(..., delta=0.1)will fail when acquisitions run longer than expected due to overhead. For timing-sensitive tests it’s typically safer to assert a minimum duration (>= expected exposure) rather than near-equality.
start = time.time()
im = self.sed.data.get()
duration = time.time() - start
self.assertEqual(im.shape, self.size[::-1])
self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration))
src/odemis/driver/test/simsem_test.py:274
- Same timing assertion problem:
assertAlmostEqual(duration, expected_duration, delta=0.1)introduces an upper bound that can be exceeded on slower machines/CI runners. Prefer checkingduration >= expected_duration(possibly with tolerance) to avoid flakiness.
start = time.time()
im = self.sed.data.get()
duration = time.time() - start
self.assertEqual(im.shape, self.size[::-1])
self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration))
src/odemis/driver/test/simsem_test.py:288
- Same timing assertion problem: with
assertAlmostEqual(..., delta=0.1)the test can fail if acquisition takes longer than expected (common under CI load). Prefer a lower-bound assertion (>= expected exposure time) to validate timing without adding a brittle upper bound.
start = time.time()
im = self.sed.data.get()
duration = time.time() - start
self.assertEqual(im.shape, self.size[::-1])
self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration))
self.assertIn(model.MD_DWELL_TIME, im.metadata)
src/odemis/driver/test/simsem_test.py:398
- Same issue in the long-dwell test:
assertAlmostEqual(duration, expected_duration, delta=0.1)will fail if runtime overhead pushes the duration above the expected dwell time. This should generally assert the acquisition wasn’t faster than expected (>=), rather than requiring near-equality.
start = time.time()
im = self.sed.data.get()
duration = time.time() - start
self.assertEqual(im.shape, self.size[::-1])
self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration))
self.assertIn(model.MD_DWELL_TIME, im.metadata)
src/odemis/driver/test/simsem_test.py:629
- Same timing assertion issue in the drift variant:
assertAlmostEqual(duration, expected_duration, delta=0.1)adds an upper bound on runtime and can make the test flaky on slower CI runners. Prefer asserting a minimum duration (>= expected exposure) instead.
self.scanner.scanPath.value = scan_path_std
start = time.time()
vector_data = self.sed.data.get()
duration = time.time() - start
self.assertAlmostEqual(duration, expected_duration, delta=0.1, msg="Error execution took %f s, expected %f s." % (duration, expected_duration))
pieleric
requested changes
May 21, 2026
…or which fails sometime
…e may depend on the screen/window decorations
The test's compute_expected_duration() included a settle time per line, but the simulator did not actually wait for it, causing flaky assertGreaterEqual failures. Add the settle time to the simulator's duration calculation, and update the test to use scanner.settleTime instead of a hardcoded value, so both stay in sync.
nandishjpatel
force-pushed
the
fix-testcase-ci-run-20260521
branch
from
e20754e to
dd83828
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/driver/test/simsem_test.py`:
- Around line 152-155: The helper method compute_expected_duration is missing a
return type hint and a reStructuredText docstring; update the method signature
to include an explicit return type (e.g., -> float) and add a concise reST
docstring describing the method, the implicit parameter self (briefly stating it
uses self.scanner), the expected types for scanner.dwellTime.value and
scanner.resolution.value (e.g., dwell: float, size: Tuple[int, int]) and what
the returned value represents (computed duration), referencing the method name
compute_expected_duration and the attributes self.scanner.dwellTime.value,
self.scanner.resolution.value, and self.scanner.settleTime so reviewers can find
the related members.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b4693f3-8444-44b4-8da5-631595897b67
📒 Files selected for processing (6)
src/odemis/driver/simsem.pysrc/odemis/driver/test/simsem_test.pysrc/odemis/driver/test/symphotime_test.pysrc/odemis/gui/test/comp_text_test.pysrc/odemis/gui/test/comp_viewport_test.pysrc/odemis/odemisd/test/start_test.py
tepals
approved these changes
May 27, 2026
nandishjpatel
force-pushed
the
fix-testcase-ci-run-20260521
branch
from
03fc35a to
c7dbc34
Compare
pieleric
approved these changes
May 29, 2026
Member
|
@nandishjpatel the changes look fine, but could you squash the last commit (pr fixes) into the 2 respective commits that it fix. I'd like to keep each fix independent from each other. |
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.
No description provided.