Skip to content

Conversation

@michelcrypt4d4mus
Copy link

I noticed that every test file was redefining path variables and a lot of the tests were working with the same sample PDFs so I did something to make the tests more in line with how pytest is designed to work. (I looked around in the issues and documentation to see if there was some reason things weren't done this way and didn't see anything).

Also theoretically with clever use of scope= in the @pytest.fixture() calls you could theoretically speed up the tests a tiny bit because there would be less object creation though I didn't attempt to do that because a) gains are likely to be small (how much time does it take to setup a PdfReader?) and b) it's a bit fraught as far as the state of the fixture objects.

@stefan6419846
Copy link
Collaborator

Thanks for the PR. Please discuss such changes first, instead of starting with a PR directly. For me, it is not obvious what the advantage of pytest fixtures is over moving the directory logic to tests/__init__.py instead, theoretically allowing for a more tool-independent approach.

@michelcrypt4d4mus
Copy link
Author

some advantages:

  1. easier to maintain the tests. if for instance you wanted to change the path to the crazyones.pdf you would now just have to update in a single place instead of in hundreds of places.
  2. pytest fixtures are composable / reusable, so see for instance crazyones_pdf_reader / crazyones_pdf_path

i didn't push it too far here because i didn't know if you guys would be interested in this change, but tbh it feels like these fixtures would be the least of your problems if you wanted to use a tool other than pytest. pytest seems to be very deeply integrated into the test suite. the changes the fixtures i put in conftest.py to constants would take a minute or two whereas getting rid of all the pytest parameterizations would be a chore.

but if you're not interested in the change feel free to close the PR.

@stefan6419846
Copy link
Collaborator

Having utility functions in tests/__init__.py instead of the fixtures would still allow for composition and reusability, thus I personally do not really see the advantages of the pytest fixtures at the moment over "normal" simplification.

i didn't push it too far here because i didn't know if you guys would be interested in this change, but tbh it feels like these fixtures would be the least of your problems if you wanted to use a tool other than pytest.

I currently do not have any plans to change this, but apart from the fact that fixtures do not require explicit imports, I do not really see any benefits of fixtures over an universal centralized regular Python solution.

@michelcrypt4d4mus
Copy link
Author

I personally do not really see the advantages of the pytest fixtures at the moment over "normal" simplification.

the advantages are not huge but they are real. in general pytest likes it when you use fixtures so there are some nice things you can do. for instance

  1. there are ways you can identify all tests that touch the sample_files_dir fixture and automatically mark them with @pytest.mark.samples rather than every test individually deciding whether it needs the sample files
  2. you can dynamically retrieve named fixture values with request.getfixturevalue(fixture_name_str)
  3. standardized setup and tear down of fixtures. instead of returning a value, a fixture can yield a return value and then do cleanup. for instance in the crazyones_pdf_reader fixture you could, rather than construct a new PdfReader, just reset the PdfReader object's seek etc. maybe a better example is that you can use fixtures to clean up any temporary files, so instead of this and this you could have those tests share a common fixture. something like:
    @pytest.fixture
    def extracted_images_dir():
       images_dir = Path("extracted-images")
       yield Path("extracted-images")
       
       for file in images_dir..glob('**/*'):
            file.unlink()

there's more stuff than that, but those are things i have found particularly useful in other contexts. either way test suites are usually neglected and i'm offering to help bring some order to the chaos, but if you're ideologically opposed to doing reusable test data the way pytest is designed to do reusable test data, then i'm sure someone else will be happy to do the test suite in the nonstandard/bespoke way you describe.

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