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

Refactor bt_list_adapters.py and add unit tests to it (BugFix) #875

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

pieqq
Copy link
Collaborator

@pieqq pieqq commented Dec 6, 2023

Description

Gave a presentation about unit testing in Checkbox. As a demo, picked a script with 0% coverage and improved its coverage.

While I initially wrote tests for the script, I realized it was a good candidate for a refactor, so I modified the script and wrote unit tests for my refactor.

Tests

  • ./manage.py test -u pass
  • I ran the script on my laptop and it prints out exactly the same data as before:
$ ./bt_list_adapters.py
rfkill3 dell-bluetooth

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8d1f256) 35.70% compared to head (cb0658c) 35.78%.
Report is 12 commits behind head on main.

Files Patch % Lines
providers/base/bin/bt_list_adapters.py 96.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
+ Coverage   35.70%   35.78%   +0.07%     
==========================================
  Files         303      303              
  Lines       34250    34262      +12     
  Branches     5917     5918       +1     
==========================================
+ Hits        12230    12260      +30     
+ Misses      21458    21439      -19     
- Partials      562      563       +1     
Flag Coverage Δ
provider-base 5.50% <96.66%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

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

A few comments here and there, the only thing that is a no-go for me is hiding code from the coverage bot by putting it in under if __name__ == "__main__", please do take those lines into a main function

providers/base/bin/bt_list_adapters.py Outdated Show resolved Hide resolved
providers/base/bin/bt_list_adapters.py Outdated Show resolved Hide resolved
providers/base/bin/bt_list_adapters.py Outdated Show resolved Hide resolved
providers/base/bin/bt_list_adapters.py Outdated Show resolved Hide resolved
providers/base/bin/bt_list_adapters.py Outdated Show resolved Hide resolved
providers/base/tests/test_bt_list_adapters.py Show resolved Hide resolved
providers/base/tests/test_bt_list_adapters.py Show resolved Hide resolved
Copy link
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

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

Nice, well done ty!

@pieqq pieqq merged commit aace0b6 into main Dec 13, 2023
14 checks passed
@pieqq pieqq deleted the revamp-bt-list-adapter-script branch December 13, 2023 14:29
binli pushed a commit to binli/checkbox that referenced this pull request Mar 22, 2024
…ical#875)

* Refactor bt_list_adapters.py and add unit tests to it

* Take Max's comments into account

* Add unit tests to cover the new main function
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.

2 participants