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: remove assert_true (master) #6157

Merged
merged 1 commit into from Jan 29, 2022

Conversation

hexagonrecursion
Copy link
Contributor

Work toward #28

This PR removes the usage of assert_true in the master branch

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2022

Codecov Report

Merging #6157 (37506ca) into master (8b61fd3) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6157      +/-   ##
==========================================
- Coverage   83.52%   83.46%   -0.06%     
==========================================
  Files          38       38              
  Lines       10380    10380              
  Branches     2037     2037              
==========================================
- Hits         8670     8664       -6     
- Misses       1208     1214       +6     
  Partials      502      502              
Impacted Files Coverage Δ
src/borg/archive.py 82.76% <0.00%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b61fd3...37506ca. Read the comment docs.

@ThomasWaldmann
Copy link
Member

did you see the comment in src/borg/testsuite/__init__.py line 24?

some of these tests are run as "borg selftest" every time borg is invoked, so they really run with the stdlib unittest to avoid a dependency on pytest. not sure how this change affects that, maybe better try it out by triggering some test failure in the self tests.

@ThomasWaldmann
Copy link
Member

Update: tried that myself:

E.g. class HashIndexDataTestCase(BaseTestCase) is part of borg.selftest and (before your changes) already uses a plain assert instead of self.assert_true().

I uninstalled pytest to avoid any interference (it does a lot of magic, including with assert), then ran borg list repo after artifically tweaking one of the test methods there to fail the assertion.

Well, it worked and the output was practically almost identical (except the missing "False is not true", which is not helpful anyway).

Thus, we can merge your PR after review.

@ThomasWaldmann ThomasWaldmann merged commit a65f298 into borgbackup:master Jan 29, 2022
@ThomasWaldmann
Copy link
Member

Thanks!

@hexagonrecursion hexagonrecursion deleted the ununit branch January 30, 2022 03:32
@hexagonrecursion
Copy link
Contributor Author

Theoretically, assert can be a no-op:
https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement

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.

None yet

3 participants