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

test: use pytest-regression in our test suit #265

Merged
merged 6 commits into from
Feb 15, 2024
Merged

test: use pytest-regression in our test suit #265

merged 6 commits into from
Feb 15, 2024

Conversation

12rambau
Copy link
Contributor

@12rambau 12rambau commented Dec 29, 2023

Fix #122

@akhmerov
Copy link
Member

akhmerov commented Dec 29, 2023

I have a bit mixed feelings about this approach.

Pros:

  • It is less code
  • It is easier to add new tests

Cons:

  • The tests compare a lot more information (the entire HTML) rather than the doctree. This may make tests fail if e.g. sphinx changes its markup.
  • Perhaps more significantly: verifying that the check is correct requires manual audit of the HTML, and is potentially error prone.

I don't think the drawbacks are severe, but I wanted to bring them up for consideration.

@12rambau
Copy link
Contributor Author

12rambau commented Dec 29, 2023

I also thought about it but I didn't find efficient ways to parse doctree nodes as a json tree which would have allowed me to continue testing the document object.

That being said, the final result is the html output and changes from sphinx needs to be captured very early by the tests to avoid breaking custom css and themes so I think it brings lots of advantages as well. It is still possible to use other builders if the html output is not sufficient or if issues are identified in pdf or epub.

At the moment the PR is not ready for review as the pre-commit hooks are conflicting with the generated files.

@12rambau 12rambau marked this pull request as ready for review January 10, 2024 13:32
@12rambau
Copy link
Contributor Author

@akhmerov do you have any last though on this one before I merge it ?
I would like to have it merged before I merge other PR to avoid the tests misalignment.

@akhmerov
Copy link
Member

Sorry for not being clear. I think this is a perfectly fine solution that removes a lot of boilerplate.

@12rambau 12rambau merged commit 5cd3ec7 into main Feb 15, 2024
10 checks passed
@12rambau 12rambau deleted the regressions branch February 15, 2024 22:32
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.

Use pytest-regressions for tests
2 participants