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

Repo tests - conversion to pytest #7626

Merged
merged 24 commits into from
Jul 10, 2023

Conversation

bigtedde
Copy link
Contributor

@bigtedde bigtedde commented Jun 6, 2023

The current repository tests are written with the unittest framework. I have began converting these to pytest, starting with just a few tests so as to make sure I am on the right track with these changes.

Very rough draft, started in a separate test file for now. If I am on the right track I will merge changes into original test file and continue converting.

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023

Codecov Report

Merging #7626 (7d41fb6) into master (a5c4d0d) will decrease coverage by 0.09%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #7626      +/-   ##
==========================================
- Coverage   83.85%   83.76%   -0.09%     
==========================================
  Files          66       66              
  Lines       11864    11900      +36     
  Branches     2149     2155       +6     
==========================================
+ Hits         9948     9968      +20     
- Misses       1346     1359      +13     
- Partials      570      573       +3     

see 10 files with indirect coverage changes

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.

Can you delete the old tests which these tests replace?

src/borg/testsuite/repo_new.py Outdated Show resolved Hide resolved
src/borg/testsuite/repo_new.py Outdated Show resolved Hide resolved
src/borg/testsuite/repo_new.py Outdated Show resolved Hide resolved
src/borg/testsuite/repo_new.py Outdated Show resolved Hide resolved
src/borg/testsuite/repo_new.py Outdated Show resolved Hide resolved
@ThomasWaldmann
Copy link
Member

Please rebase on current master.

src/borg/repository.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
src/borg/repository.py Outdated Show resolved Hide resolved
src/borg/repository.py Outdated Show resolved Hide resolved
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.

guess that fixture/contextmanager combination is problematic.

src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
def test_basic_operations(fixture, request):
repo = request.getfixturevalue(fixture)
with repo as repository:
@pytest.mark.parametrize("fixture", [pytest.lazy_fixture("repository"), pytest.lazy_fixture("remote_repository")])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works, however when discussing with @m3nu we were skeptical about including this as it seems like a small project that could be compromised: https://github.com/TvoroG/pytest-lazy-fixture

Copy link
Contributor

Choose a reason for hiding this comment

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

Or abandoned and unmaintained, which sadly happens too often for useful packages.

Copy link
Member

Choose a reason for hiding this comment

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

That lazy-fixture project doesn't look like regularly maintained. Last release 3.5y old.

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.

please review these "corruption tests" concerning the correct opening, reopening, closing of the repo, where a commit/rollback happens, that stuff that does not need to be within the repo context is happening outside the repo context.

@bigtedde
Copy link
Contributor Author

please review these "corruption tests" concerning the correct opening, reopening, closing of the repo, where a commit/rollback happens, that stuff that does not need to be within the repo context is happening outside the repo context.

I am reviewing this now, and will have this taken care of by end of day 👍

@bigtedde
Copy link
Contributor Author

Hmmm. Windows test didn't pass -

borg.testsuite.archiver.extract_cmd.ArchiverTestCase testMethod=test_sparse_file

It provides assertion error that looks similar to one encountered the other day (something to do with a Location object, iirc)

with open(filename, "rb") as fd:
                # check if file contents are as expected
>               self.assert_equal(fd.read(hole_size), b"\0" * hole_size)
E               AssertionError: b'\x0[8388600 chars]0\x00|\xbe \xe0\xc1\xfcg\xbf\xf1bGF\x931\x8c\x[159271271 chars]\x00' != b'\x0[8388600 chars]0\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\[159383507 chars]\x00'

I looked through the test_spare_file test and couldn't find anything that would have broken from my changes 🤔. Will keep my eyes peeled.

@ThomasWaldmann
Copy link
Member

@bigtedde

Hmmm. Windows test didn't pass -
borg.testsuite.archiver.extract_cmd.ArchiverTestCase testMethod=test_sparse_file

see #7616.

@bigtedde
Copy link
Contributor Author

bigtedde commented Jun 20, 2023

Made some minor clean up changes today, and after a new commit they got past the spurious Windows tests error.

@ThomasWaldmann I am feeling really good about where we are with this test conversion. Do you have any areas of concern I could address before marking this PR ready for review?

src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
@bigtedde bigtedde marked this pull request as ready for review June 23, 2023 18:11
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.

ok, last review, will merge after final fixes.

src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
src/borg/testsuite/repository.py Outdated Show resolved Hide resolved
@ThomasWaldmann
Copy link
Member

squash all commits together and keep what looks valuable from all the commit comments?

@ThomasWaldmann ThomasWaldmann merged commit 33645ad into borgbackup:master Jul 10, 2023
11 checks passed
@ThomasWaldmann
Copy link
Member

Thanks!

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.

5 participants