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

Archiver folder - all tests converted from unittest to pytest #7722

Merged
merged 55 commits into from
Jul 23, 2023

Conversation

bigtedde
Copy link
Contributor

@bigtedde bigtedde commented Jul 9, 2023

In this PR, I removed the ArchiverTestCaseBase and all unittest elements from the 32 test files in the archiver folder of the testsuite, converting everything to the pytest framework. I will continue to work on review and refinement.

Any and all feedback is greatly appreciated!

@ThomasWaldmann @m3nu

conftest.py Outdated Show resolved Hide resolved
src/borg/testsuite/archiver/checks.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
@bigtedde
Copy link
Contributor Author

@ThomasWaldmann I think I was misunderstanding you before. I was under the impression you just wanted prefix removed from ArchiverSetup and left in the remote_archiver setup. If you want prefix removed entirely, I think this can be done - everywhere I am currently checking the prefix, I can instead just check if archiver.repository_location.startswith("ssh://__testsuite__")

conftest.py Outdated Show resolved Hide resolved
conftest.py Show resolved Hide resolved
@ThomasWaldmann
Copy link
Member

OK, guess this can be merged soon? The one minor thing I found right now plus the borg.exe testing, anything else?

@bigtedde
Copy link
Contributor Author

bigtedde commented Jul 23, 2023

OK, guess this can be merged soon? The one minor thing I found right now plus the borg.exe testing, anything else?

I just pushed the remote_repo boolean check change. Also, with a bit of assistance from Manu I was able to test on the binary and all tests passed without any unexpected behavior.

I do believe these changes are good to merge with your approval. There are the same number of tests, they cover the same amount of code, and they all pass as expected. All the unittest elements have been removed and replaced with pytest, and I believe these changes have left the archiver test suite easier to read and build upon going forward.

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for refactoring this!

@ThomasWaldmann ThomasWaldmann merged commit e1cd38a into borgbackup:master Jul 23, 2023
11 checks passed
@ThomasWaldmann
Copy link
Member

@bigtedde can you try that now? #7722 (comment)

@bigtedde
Copy link
Contributor Author

A lot of (all?) code is calling this with something like f"--repo={archiver.repo_location}" as first argument.

Considering that we have archiver here, that seems a bit too complicated and redundant as we could also create that argument here.

I searched though the directory and can confirm that all cmd calls are with f"--repo={archiver.repo_location}" as the first argument. It is redundant, and I can create that argument in the cmd function itself.

@bigtedde
Copy link
Contributor Author

LGTM, thanks for refactoring this!

Thank you for your help and mentoring along the way Thomas. It means a lot to me, and I am very excited to have this merged in!

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.

3 participants