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

Disable unit testing #234

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Conversation

bessman
Copy link
Collaborator

@bessman bessman commented Aug 22, 2024

This pull request removes the ability to run the tests as unit tests, by removing the code for recording and replaying serial traffic.

The tests remain useful as integration tests, but the maintenance overhead of re-recording serial traffic for every little change in order for the unit tests to pass has proven prohibitive.

Summary by Sourcery

Disable unit testing by removing the code for recording and replaying serial traffic, transitioning tests to function as integration tests. Update test fixtures accordingly and remove the test job from the CI workflow.

Enhancements:

  • Remove the ability to run tests as unit tests by eliminating the code for recording and replaying serial traffic.
  • Simplify the test setup by removing the need for mock handlers and recorded traffic.

CI:

  • Remove the test job from the CI workflow, which previously ran tests across multiple Python versions.

Tests:

  • Update test fixtures to use real SerialHandler instances instead of MockHandler, reflecting the shift from unit to integration testing.

Chores:

  • Delete JSON files used for recording serial traffic during unit tests.

Copy link

sourcery-ai bot commented Aug 22, 2024

Reviewer's Guide by Sourcery

This pull request removes the ability to run tests as unit tests by eliminating the code for recording and replaying serial traffic. The tests will now only function as integration tests, requiring a real PSLab device to be connected. The changes primarily affect the test infrastructure and how tests are executed, without modifying the core functionality of the PSLab library itself.

File-Level Changes

Files Changes
pslab/serial_handler.py
tests/conftest.py
Removed the ability to run tests as unit tests by eliminating code for recording and replaying serial traffic
tests/test_logic_analyzer.py
tests/test_oscilloscope.py
tests/test_spi.py
tests/test_uart.py
tests/test_i2c.py
tests/test_multimeter.py
tests/test_waveform_generator.py
tests/test_motor.py
tests/test_power_supply.py
Simplified test setup by removing conditional logic for unit vs. integration testing
tests/test_logic_analyzer.py
tests/test_oscilloscope.py
tests/test_spi.py
tests/test_uart.py
tests/test_i2c.py
tests/test_cli.py
tests/test_motor.py
Updated test documentation to reflect that tests now require a connected PSLab device
pslab/serial_handler.py Removed MockHandler class and related code from serial_handler.py
tests/recordings/ Deleted all JSON recording files used for unit testing
.github/workflows/workflow.yml Removed test job from GitHub Actions workflow
tests/test_logic_analyzer.py
tests/test_oscilloscope.py
tests/test_spi.py
tests/test_uart.py
tests/test_i2c.py
tests/test_multimeter.py
tests/test_waveform_generator.py
tests/test_motor.py
tests/test_power_supply.py
Updated various test files to remove logging and conditional logic related to unit testing

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @bessman - I've reviewed your changes - here's some feedback:

Overall Comments:

  • While simplifying the testing approach is understandable, completely removing unit tests in favor of integration tests only could make development and CI more challenging. Consider keeping a small set of critical unit tests with recorded data to maintain some level of quick, hardware-independent testing.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 7 issues found
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

tests/test_logic_analyzer.py Show resolved Hide resolved
tests/test_oscilloscope.py Show resolved Hide resolved
tests/test_spi.py Show resolved Hide resolved
tests/test_uart.py Show resolved Hide resolved
tests/test_multimeter.py Show resolved Hide resolved
tests/test_waveform_generator.py Show resolved Hide resolved
tests/test_i2c.py Show resolved Hide resolved
@bessman
Copy link
Collaborator Author

bessman commented Aug 22, 2024

While simplifying the testing approach is understandable, completely removing unit tests in favor of integration tests only could make development and CI more challenging. Consider keeping a small set of critical unit tests with recorded data to maintain some level of quick, hardware-independent testing.

This is a good idea but will be done in a separate pull request. The old method of recording and replaying serial traffic is obsolete, and should be removed in favor of pytest-reserial anyway.

@bessman bessman force-pushed the refactor/remove-tests branch from 0d06012 to 7cc9691 Compare August 22, 2024 12:32
@bessman bessman merged commit 6e52888 into fossasia:development Aug 22, 2024
2 checks passed
@bessman bessman deleted the refactor/remove-tests branch August 22, 2024 12:36
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.

1 participant