Readable test diffs with pytest-icdiff

[PeterNerlich]

Short description

This PR makes the output of failed API tests legible, by producing deep diffs of the JSON responses. It is possible to quickly see which values differ from the expected output.

Proposed changes

• Add pytest-diff as dependency (no additional setup seems to be required)

Side effects

• Tests are now producing different output. I hope this is not being parsed after the fact somewhere? (since pytest-diff is a plugin for pytest, pytest itself should not have issues)

Resolved issues

Fixes: #1953

---

Pull Request Review Guidelines

[codeclimate]

Code Climate has analyzed commit 6db2b41 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 75.3% (0.0% change).

View more on Code Climate.

[JoeyStk]

Thank you so much for your PR. I had a little read on pytest-diff, and as for me we can try this out. But it would be good if someone with more experience on this could also have a look :)

[timobrembeck]

Thanks! 👍
In general I agree that the current test output isn't ideal and I'm fine with changing it (and we also don't parse this anywhere).

I'm not sure what I'm doing wrong, but somehow I cannot get this to work... I just changed a few small details in the expected API output, and the error still looks very unreadable:

>           assert result == response.json()
E           AssertionError: assert
E             [{'id': 35, 'url': 'http://localhost:8000/augsburg/de/behörden-und-beratung/behörden/', 'path': '/augsburg/de/behörden-und-beratung/behörden/', 'title': 'Behörden', 'modified_gmt': '2022-01-16T16:36:57.038Z', 'last_updated': '2022-01-16T17:36:57.038+01:00', 'excerpt': '', 'content': '', 'parent': {'id': 14, 'url': 'http://localhost:8000/augsburg/de/behörden-und-beratung/', 'path': '/augsburg/de/behörden-und-beratung/'}, 'order': 2, 'available_languages': {'en': {'id': 47, 'url': 'http://localhost:8000/augsburg/en/authorities-and-counselling/public-authorities/', 'path': '/augsburg/en/authorities-and-counselling/public-authorities/'}...
E
E             ...Full output truncated (11 lines hidden), use '-vv' to show

tests/api/test_api_result.py:46: AssertionError

And I'm pretty sure it looked exactly the same before. But maybe I have a different problem, does it work for you?
Also, I think the dependency itself isn't a good choice, mainly for these reasons:

1. It's only version number 0.1.14 and hasn't been updated for over three years, so probably it died during early development stage
2. The author of the package either deleted or renamed their GitHub username (https://github.com/username/pytest-diff isn't reachable anymore), so not sure where any maintenance would happen... If you think we should use this dependency nevertheless, I'd suggest you download the source code, create a new repository under the Digitalfabrik orga and we start maintaining this dependency ourselves. But probably this isn't worth the effort, maybe you could do a little bit of research to find a different package? Or maybe simply adding -vv as the error message suggests works as well?

Apart from that, the dev dependencies also need to be added to the Pipfile.lock, only adding them to the Pipfile leads to an inconsistent state and means that we don't install the same dependency versions on all machines anymore.

So please run pipenv install --dev <package-name> as described in the docs to update the lockfile as well, thanks!

[svenseeberg]

Considering the state of pytest-diff, do you intend to continue with this PR? Otherwise we can probably close it and revisit the issue another time?

[PeterNerlich]

Thank you for your patience! I did a bit more research and compiled some screenshots.

For the sake of comparison, this is how it looks currently:

---

And this is how it looks with pytest-diff – if you indeed pass -vv to pytest as well (it first regurgitates the whole strings, it looks like):

I guess when we tried it, Ingo was running individual tests and using -vv anyway to cut down on waiting time, so we didn't notice. The following options also require -vv to be helpful, but there is an issue and a PR upstream that would separate that out to its own setting.

---

But, pytest-diff still is of little help if the individual lines are very long, like in tests/api/expected_outputs/, which was what originally motivated this search. Here pytest-icdiff seems pretty cool:



This displays the versions side by side in two columns, which might be annoying if the detected width is not your terminal width (e.g. when viewing the Circle CI logs) and is not my first preference (I like the way GitHub highlights differences). But it cuts down on the clutter very nicely. Additions, changes and removals are highlighted on a character level.

---

Lastly, there is pytest-clarity:



This makes it a bit easier to spot the changes since lines with changes are printed in color with the concrete changed characters inverted, but it displays the whole thing – I left out way more from this than from the screenshots of pytest-icdiff.

---

My personal verdict is that I like pytest-icdiff better than the other presented options, including the option of not changing anything.

What I have not been able to figure out is how exactly running pytest as part of the dev-tools/test.sh is different from running e.g.

pipenv run pytest tests/api/test_api_result.py::test_api_result[/api/augsburg/de/pages/-/augsburg/de/wp-json/extensions/v3/pages/-tests/api/expected-outputs/augsburg_de_pages.json-200] -s --disable-warnings --ds=integreat_cms.core.docker_settings

directly. Running dev-tools/test.sh somehow shows the whole diff in red in addition to apparently not being able to identify the terminal width:

If you like the approach and think figuring out the last part won't take too much effort, I'll continue with this, otherwise we can close this.

[timobrembeck]

Thanks for revisiting this! 👍

My personal verdict is that I like pytest-icdiff better than the other presented options, including the option of not changing anything.

I agree, this tools looks very promising!

Running dev-tools/test.sh somehow shows the whole diff in red in addition to apparently not being able to identify the terminal width

Both problems seems to be caused by an incompatibility between pytest-icdiff and pytest-xdist. I suggest to:

• Provide a workaround to pass an additional option to the test.sh tool to disable the parallel execution to improve the output (run without the --numprocesses=auto flag).
• If you can find out which package is the culprit, you can open an issue for it. Or open issues in both repositories and leave it up to the maintainers to figure this out.

[PeterNerlich]

Found out that getting the colored output seems to be as easy as adding --color=yes to the pytest arguments. Improving the columns is not as easy and I will probably be submitting a PR to the project allowing for all of the arguments in icdiff to also be specified for pytest-icdiff.

I'd also like to wait until #2027 is merged

[PeterNerlich]

I think I found a quite workable solution for now. Running ./dev-tools/test.sh as usual now includes a properly coloured icdiff, through it thinks the terminal is 80 characters wide due to xdist specifics. For easy diffs with singular diffs or where the changes are near the start, this already helps a lot:

But sometimes the output is cut before the actual changes, when icdiff is still only showing the context.
In that case, one can now actually follow the instructions and add -vv to get verbose output (specifically, I made sure to pass on all levels from -v to -vvvv while I was at it). In that case, I opted to also disable parallel tests using xdist, meaning the tests will take more time, but the output won't be overloaded with xdist specific verbose messages in addition to the diffs utilising the full width of the terminal:

[timobrembeck]

Thanks a lot, makes sense! 👍

Could you also document the new verbose flags and mention that they implicate performance drawbacks?

[timobrembeck]

Thanks! 👍

[JoeyStk]

LGTM