Switch to integration tests over radio#382
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughCI and Makefile now require an explicit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 352-357: The gds-integration Makefile target currently falls back
to /dev/ttyBOARD; change it to fail fast when UART_DEVICE is empty by checking
$(UART_DEVICE) and if unset print a clear error (e.g., "UART_DEVICE is required
for gds-integration") to stderr and exit non‑zero instead of using
/dev/ttyBOARD; keep using $(GDS_COMMAND) --gui=none --uart-device $(UART_DEVICE)
when provided and remove the silent default reference to /dev/ttyBOARD so CI
must supply UART_DEVICE explicitly.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0fbbd2b7-7fc0-4513-a3ab-361448c41cc5
📒 Files selected for processing (2)
.github/workflows/ci.yamlMakefile
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PROVESFlightControllerReference/test/int/rtc_test.py`:
- Around line 314-323: The test test_06_rtc_alarm_cancellation contradicts its
docstring: after sending ALARM_SET then ALARM_CANCEL it currently calls
fprime_test_api.await_event(f"{rtcManager}.AlarmTriggered") which waits for the
alarm to fire; replace that positive wait with a negative assertion (e.g., call
the test framework's event-count assertion such as
fprime_test_api.assert_event_count(f"{rtcManager}.AlarmTriggered", count=0,
timeout=10) or an equivalent "assert no events" helper) so the test verifies the
AlarmTriggered event does NOT occur after cancellation; update the call
referencing fprime_test_api.await_event accordingly and keep the preceding
proves_send_and_assert_command(fprime_test_api, f"{rtcManager}.ALARM_CANCEL")
unchanged.
- Line 336: The ALARM_CANCEL calls to proves_send_and_assert_command are missing
the required ID parameter and/or passing it with the wrong type; update both
calls invoking f"{rtcManager}.ALARM_CANCEL" to include the mandatory ID argument
and pass it as a string (e.g., "0") so args is a list[str] — ensure the call
signatures to proves_send_and_assert_command include args like ["0"] for the
ALARM_CANCEL command.
🪄 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: 89aa30af-4041-4051-9aaa-97f93ad389b6
📒 Files selected for processing (1)
PROVESFlightControllerReference/test/int/rtc_test.py
| proves_send_and_assert_command( | ||
| fprime_test_api, | ||
| f"{rtcManager}.ALARM_SET", | ||
| [alarm_time_data_str], | ||
| ) | ||
|
|
||
| # Cancel the alarm immediately | ||
| fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL") | ||
| proves_send_and_assert_command(fprime_test_api, f"{rtcManager}.ALARM_CANCEL") | ||
|
|
||
| fprime_test_api.await_event(f"{rtcManager}.AlarmTriggered", timeout=10) |
There was a problem hiding this comment.
Test logic contradicts the docstring.
The test is named test_06_rtc_alarm_cancellation and the docstring states it should verify that "it does not trigger", but line 323 uses await_event which waits for AlarmTriggered to occur. This appears to test the opposite of what's documented.
If the intent is to verify the alarm does not trigger after cancellation, you should use assert_event_count with count=0 or a similar negative assertion pattern.
🐛 Proposed fix to verify alarm does NOT trigger
# Cancel the alarm immediately
proves_send_and_assert_command(fprime_test_api, f"{rtcManager}.ALARM_CANCEL")
- fprime_test_api.await_event(f"{rtcManager}.AlarmTriggered", timeout=10)
+ # Wait past when the alarm would have triggered, then verify no AlarmTriggered event
+ time.sleep(7) # Wait longer than the 5-second alarm delay
+ triggered_events = fprime_test_api.get_event_test_history().retrieve(
+ fprime_test_api.get_event_pred(f"{rtcManager}.AlarmTriggered")
+ )
+ assert len(triggered_events) == 0, "Alarm should not have triggered after cancellation"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PROVESFlightControllerReference/test/int/rtc_test.py` around lines 314 - 323,
The test test_06_rtc_alarm_cancellation contradicts its docstring: after sending
ALARM_SET then ALARM_CANCEL it currently calls
fprime_test_api.await_event(f"{rtcManager}.AlarmTriggered") which waits for the
alarm to fire; replace that positive wait with a negative assertion (e.g., call
the test framework's event-count assertion such as
fprime_test_api.assert_event_count(f"{rtcManager}.AlarmTriggered", count=0,
timeout=10) or an equivalent "assert no events" helper) so the test verifies the
AlarmTriggered event does NOT occur after cancellation; update the call
referencing fprime_test_api.await_event accordingly and keep the preceding
proves_send_and_assert_command(fprime_test_api, f"{rtcManager}.ALARM_CANCEL")
unchanged.
|
|
||
| # validate that cancel doesn't work without an alarm being present | ||
| fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL", [0]) | ||
| proves_send_and_assert_command(fprime_test_api, f"{rtcManager}.ALARM_CANCEL", [0]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the ALARM_CANCEL command definition to understand expected arguments
rg -n "ALARM_CANCEL" --type-add 'fpp:*.fpp' --type=fpp -A5 -B2Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 1048
🏁 Script executed:
# Check the actual test file around lines 321 and 336
sed -n '315,345p' PROVESFlightControllerReference/test/int/rtc_test.pyRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 1086
🏁 Script executed:
# Find and examine the proves_send_and_assert_command function definition
rg -n "def proves_send_and_assert_command" -A 10Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 1118
ALARM_CANCEL missing required argument and type mismatch.
The ALARM_CANCEL command requires a mandatory ID: U16 parameter (per RtcManager.fpp). However, line 321 calls it without any arguments, which will fail. Additionally, line 336 attempts to provide the argument as [0], but the function signature expects args: list[str], making this a type mismatch (integer vs string).
Context
Line 321:
proves_send_and_assert_command(fprime_test_api, f"{rtcManager}.ALARM_CANCEL")
Line 336:
proves_send_and_assert_command(fprime_test_api, f"{rtcManager}.ALARM_CANCEL", [0])
Command definition in RtcManager.fpp:
sync command ALARM_CANCEL(
ID: U16 @< ID of the alarm to cancel
)
Function signature:
def proves_send_and_assert_command(
fprime_test_api: IntegrationTestAPI,
command: str,
args: list[str] = [],
...
)
Both calls need fixing: line 321 needs the argument added, and line 336 needs the argument as a string (e.g., ["0"]).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PROVESFlightControllerReference/test/int/rtc_test.py` at line 336, The
ALARM_CANCEL calls to proves_send_and_assert_command are missing the required ID
parameter and/or passing it with the wrong type; update both calls invoking
f"{rtcManager}.ALARM_CANCEL" to include the mandatory ID argument and pass it as
a string (e.g., "0") so args is a list[str] — ensure the call signatures to
proves_send_and_assert_command include args like ["0"] for the ALARM_CANCEL
command.
2 participants
Switch to integration tests over radio
Description
We've changed the physical setup for the CI machine to now use a radio passthrough board to communicate with the CI cube instead of communicating through UART. This will allow us to continuously test radio functionality, as well as in the future add some radio specific tests.
Related Issues/Tickets
Closes #59
How Has This Been Tested?
Will be tested by seeing if integration works on this PR
Screenshots / Recordings (if applicable)
Checklist
Further Notes / Considerations