-
Notifications
You must be signed in to change notification settings - Fork 229
Fix typo FERQUENCY -> FREQUENCY and resolve TypeError in tests #263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideTests for SPI and UART were updated to fix a misspelled PWM frequency constant and to replace incorrect, method-based frequency calculations with a simple static test frequency to avoid a TypeError and stabilize timing expectations. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- Instead of replacing the frequency expressions with a hard-coded 1000, consider fixing the underlying issue by using the correct API (e.g., calling
SPIMaster._frequency()/UART._baudrateappropriately or exposing a public attribute) so the tests still reflect the actual configuration logic. - If a fixed test frequency is intentional, it would be clearer to derive it from the relevant SPI/UART parameters or define a single shared constant for PWM test frequency to avoid divergence from the real hardware values in multiple places.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of replacing the frequency expressions with a hard-coded 1000, consider fixing the underlying issue by using the correct API (e.g., calling `SPIMaster._frequency()` / `UART._baudrate` appropriately or exposing a public attribute) so the tests still reflect the actual configuration logic.
- If a fixed test frequency is intentional, it would be clearer to derive it from the relevant SPI/UART parameters or define a single shared constant for PWM test frequency to avoid divergence from the real hardware values in multiple places.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thanks for the feedback! Since we cannot call the instance method at this scope, I replaced it with a fixed value (1000) to ensure the test suite passes without crashing. This value is sufficient for the mocked tests. |
Description
This PR fixes a typo in
tests/test_spi.pyandtests/test_uart.pywherePWM_FREQUENCYwas misspelled asPWM_FERQUENCY.Changes
PWM_FERQUENCYtoPWM_FREQUENCY.TypeErrorcaused by the original assignment logic (SPIMaster._frequency * 2 / 3). Since_frequencyis a method, multiplying it directly caused a crash during testing.1000) which is sufficient for these test cases.Verification
Ran local tests using
pytest tests/test_spi.py tests/test_uart.pyand confirmed they no longer raiseTypeError.Summary by Sourcery
Fix SPI and UART test configurations to use a correctly named PWM frequency constant and avoid runtime type errors.
Bug Fixes:
Tests: