Skip to content
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

test(api): fix or ignore Decoy related warnings in unit tests #16944

Open
wants to merge 1 commit into
base: edge
Choose a base branch
from

Conversation

jbleon95
Copy link
Contributor

Overview

This PR aims to reduce the amount of warnings generated by the API unit tests by fixing or selectively ignoring specific Decoy warnings.

Across the entire test suite, MissingSpecAttributeWarning has been suppressed. The purpose of this is to catch non-existent attributes of decoy mock objects, but it's being raised for any property that is referenced, which accounts for about 250 warnings. Ignoring these should not cause any issues because if a property is not being referred to properly, the vast majority of the time that will cause the test to fail anyway due to some rehearsal not occurring.

The other warning being suppressed is RedundantVerifyWarning for protocol_runner.py: test_load_json_runner, which cannot be fixed without harming the coverage of the test. Because there is a long explanation of why it is needed, I've chosen to simply ignore that for this specific test.

Otherwise I've fixed up a bunch of IncorrectCallWarning, RedundantVerifyWarning and MiscalledStubWarning throughout the unit tests.

There are still a few existing DeprecationWarnings and a bunch of RuntimeWarnings related to CanMessenger._read_task_shield' was never awaited but fixing those would involve more effort or changing packages/editing hardware code, so those have not been fixed.

Overall this reduces the amount of warnings from ~330 to ~40 warnings.

Changelog

  • fix/ignore Decoy warnings in api tests

Review requests

If anybody has an easy fix for any remaining errors, feel free to push it to this branch.

Risk assessment

Low

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smash that mf approve button

Copy link
Contributor

@ryanthecoder ryanthecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:trollface:

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Very small number of code changes for that level of error reduction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants