Rtc test fix#407
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRTC integration tests switch alarm event checks from await_event(..., timeout=10) to fprime_test_api.assert_event(..., start="NOW", timeout=10) and adjust double-set alarm scheduling times; the Zephyr workspace subproject pointer is advanced to a new commit. ChangesRTC Test Event Assertion Migration
Zephyr Subproject Pointer Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 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. 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: 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 `@PROVESFlightControllerReference/test/int/rtc_test.py`:
- Around line 309-311: The test test_06_rtc_alarm_cancellation is asserting that
f"{rtcManager}.AlarmTriggered" is emitted after canceling an alarm, which
contradicts the test purpose; update the assertion logic in
test_06_rtc_alarm_cancellation so that after calling the cancel flow you either
assert an AlarmCanceled (or equivalent) event from f"{rtcManager}" if such an
event exists, or assert that f"{rtcManager}.AlarmTriggered" is NOT emitted (use
the framework's "not emitted"/absence check with an appropriate timeout), or
alternatively query the ALARM_LIST (as done earlier near line ~283) to verify
the alarm is removed. Ensure references to rtcManager, AlarmTriggered,
AlarmCanceled (if present), and ALARM_LIST in the test are used to locate and
implement the correct check.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: d079521f-fc18-4658-a63d-c8095bbf96f9
📒 Files selected for processing (2)
PROVESFlightControllerReference/test/int/rtc_test.pylib/zephyr-workspace/zephyr
| fprime_test_api.assert_event( | ||
| f"{rtcManager}.AlarmTriggered", start="NOW", timeout=10 | ||
| ) |
There was a problem hiding this comment.
Test logic contradicts its purpose.
The test test_06_rtc_alarm_cancellation has a docstring stating "Test that we can cancel an RTC alarm and that it does not trigger", but the assertion checks that AlarmTriggered event is emitted after canceling the alarm. This is logically inconsistent.
After canceling an alarm, the test should either:
- Assert that an
AlarmCanceledor similar confirmation event is emitted (if one exists) - Verify the alarm does NOT trigger by checking that
AlarmTriggeredis not emitted - Verify the alarm is no longer set using
ALARM_LIST(similar to line 283)
🔍 Suggested fix - verify alarm does not trigger
# Cancel the alarm immediately
fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL")
-fprime_test_api.assert_event(
- f"{rtcManager}.AlarmTriggered", start="NOW", timeout=10
-)
+# Verify the alarm is no longer set
+fprime_test_api.send_command(f"{rtcManager}.ALARM_LIST")
+fprime_test_api.assert_event(f"{rtcManager}.AlarmNotSet", start="NOW", timeout=10)Alternatively, if there's an AlarmCanceled event, assert that instead.
🤖 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 `@PROVESFlightControllerReference/test/int/rtc_test.py` around lines 309 - 311,
The test test_06_rtc_alarm_cancellation is asserting that
f"{rtcManager}.AlarmTriggered" is emitted after canceling an alarm, which
contradicts the test purpose; update the assertion logic in
test_06_rtc_alarm_cancellation so that after calling the cancel flow you either
assert an AlarmCanceled (or equivalent) event from f"{rtcManager}" if such an
event exists, or assert that f"{rtcManager}.AlarmTriggered" is NOT emitted (use
the framework's "not emitted"/absence check with an appropriate timeout), or
alternatively query the ALARM_LIST (as done earlier near line ~283) to verify
the alarm is removed. Ensure references to rtcManager, AlarmTriggered,
AlarmCanceled (if present), and ALARM_LIST in the test are used to locate and
implement the correct check.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
PROVESFlightControllerReference/test/int/rtc_test.py (1)
327-329:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTest logic still contradicts its purpose.
The test cancels an alarm but then asserts that
AlarmTriggeredis emitted, which contradicts the docstring "that it does not trigger". As noted in the previous review, after canceling you should assertAlarmCanceledor verifyAlarmTriggeredis not emitted.🤖 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 `@PROVESFlightControllerReference/test/int/rtc_test.py` around lines 327 - 329, The test currently cancels an alarm but then asserts that f"{rtcManager}.AlarmTriggered" is emitted, which contradicts the docstring; update the assertion after the cancel operation to either assert that f"{rtcManager}.AlarmCanceled" is emitted (use fprime_test_api.assert_event with that event name and suitable start/timeout) or verify that f"{rtcManager}.AlarmTriggered" is NOT emitted (use the test API's non-emission check or invert the assert_event call) so the test verifies cancellation rather than a trigger.
🤖 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 `@PROVESFlightControllerReference/test/int/rtc_test.py`:
- Around line 425-432: Replace the dict(...) construction used to build
alarm_time_data with a dictionary literal for consistency and style: locate the
alarm_time_data assignment in rtc_test.py (variable name alarm_time_data) and
rewrite it using a brace-enclosed literal with the same keys (Year, Month, Day,
Hour, Minute, Second) and values (alarm_time.year, alarm_time.month, etc.),
preserving ordering and semantics.
---
Duplicate comments:
In `@PROVESFlightControllerReference/test/int/rtc_test.py`:
- Around line 327-329: The test currently cancels an alarm but then asserts that
f"{rtcManager}.AlarmTriggered" is emitted, which contradicts the docstring;
update the assertion after the cancel operation to either assert that
f"{rtcManager}.AlarmCanceled" is emitted (use fprime_test_api.assert_event with
that event name and suitable start/timeout) or verify that
f"{rtcManager}.AlarmTriggered" is NOT emitted (use the test API's non-emission
check or invert the assert_event call) so the test verifies cancellation rather
than a trigger.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 4d9460cb-603f-4f23-8e8c-e12f2d58d72f
📒 Files selected for processing (1)
PROVESFlightControllerReference/test/int/rtc_test.py
| alarm_time_data = dict( | ||
| Year=alarm_time.year, | ||
| Month=alarm_time.month, | ||
| Day=alarm_time.day, | ||
| Hour=alarm_time.hour, | ||
| Minute=alarm_time.minute, | ||
| Second=alarm_time.second, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider using dict literal for consistency.
Static analysis suggests replacing dict(...) with a dictionary literal {...} for consistency with Python conventions. This is a minor style improvement.
✨ Optional refactor to dict literal
- alarm_time_data = dict(
- Year=alarm_time.year,
- Month=alarm_time.month,
- Day=alarm_time.day,
- Hour=alarm_time.hour,
- Minute=alarm_time.minute,
- Second=alarm_time.second,
- )
+ alarm_time_data = {
+ "Year": alarm_time.year,
+ "Month": alarm_time.month,
+ "Day": alarm_time.day,
+ "Hour": alarm_time.hour,
+ "Minute": alarm_time.minute,
+ "Second": alarm_time.second,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| alarm_time_data = dict( | |
| Year=alarm_time.year, | |
| Month=alarm_time.month, | |
| Day=alarm_time.day, | |
| Hour=alarm_time.hour, | |
| Minute=alarm_time.minute, | |
| Second=alarm_time.second, | |
| ) | |
| alarm_time_data = { | |
| "Year": alarm_time.year, | |
| "Month": alarm_time.month, | |
| "Day": alarm_time.day, | |
| "Hour": alarm_time.hour, | |
| "Minute": alarm_time.minute, | |
| "Second": alarm_time.second, | |
| } |
🧰 Tools
🪛 Ruff (0.15.14)
[warning] 425-432: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🤖 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 `@PROVESFlightControllerReference/test/int/rtc_test.py` around lines 425 - 432,
Replace the dict(...) construction used to build alarm_time_data with a
dictionary literal for consistency and style: locate the alarm_time_data
assignment in rtc_test.py (variable name alarm_time_data) and rewrite it using a
brace-enclosed literal with the same keys (Year, Month, Day, Hour, Minute,
Second) and values (alarm_time.year, alarm_time.month, etc.), preserving
ordering and semantics.
2 participants
Functionality remains the same but it looks like it is more common convention to use assert instead
Pull Request Title (e.g., Feature: Add user authentication)
Description
Related Issues/Tickets
How Has This Been Tested?
Screenshots / Recordings (if applicable)
Checklist
Further Notes / Considerations