Add unit tests for render engines

[tomers]

Adds initial framework to run pytest tests. Test text render engine.

[maresb]

Woah, this is really awesome!!!

I know from the draft state that you're not done. My main thought is that we shouldn't write files into the repo; instead we should use the tmp_path fixture.

My other thought is that I wonder if there's a simple way to automate regenerating the images (e.g. if we change the default font), but that's less important.

[maresb]

Ah, and because of Python import weirdness, you should add import PIL.ImageOps in addition to import PIL at the top of test_render_engines.py.

[maresb]

@tomers, you should probably rebase this on the latest main

[maresb]

I don't like running pytest in pre-commit. Individual commits should be able to break tests. For example, this is desirable in test-driven development where you first commit a broken test and then fix it in the subsequent commits.

[tomers]

You can run pre-commit with --no-verify. https://pre-commit.com/#temporarily-disabling-hooks

I prefer to run as much as possible in pre-commit, as long as it doesn't take too long.

On Mon, Apr 29, 2024, 12:57 Ben Mares @.***> wrote:

I don't like running pytest in pre-commit. Individual commits should be able to break tests. For example, this is desirable in test-driven development where you first commit a broken test and then fix it in the subsequent commits.

— Reply to this email directly, view it on GitHub https://github.com/labelle-org/labelle/pull/32#issuecomment-2082319334, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUL47SM6GZ7W5HVTRXPGLY7YKP5AVCNFSM6AAAAABG5C3X3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBSGMYTSMZTGQ . You are receiving this because you were mentioned.Message ID: @.***>

[maresb]

I rely on pre-commit for linting and type-checking which I find essential for lowering my commit entropy.

Including pytest will encourage developers to make larger commits which makes review difficult.

Also, I can't think of a single major open-source project that runs pytest in pre-commit. I think there are good reasons for that.

[tomers]

My main thought is that we shouldn't write files into the repo; instead we should use the tmp_path fixture.

I prefer not to. The reason for this is that:

1. I delete each created file when the test succeed
2. In case of test failure, the file is in the directory just above the reference expected_renders directory, thus it is easy to compare. In case the new file is the correct one (e.g. following a code change), we can just move the file into that directory. It is very easy to do from the IDE, where the actual image file is near the code, the test code, and the reference image file, and by simple and short drag-and-drop action it can be moved to the expected_renders directory, and be committed.

My other thought is that I wonder if there's a simple way to automate regenerating the images (e.g. if we change the default font), but that's less important.

I don't think we should. I prefer to do it manually, and examine each change. As I wrote above, we can just drag and drop in order to move the new version of the file to its designated place.

I rely on pre-commit for linting and type-checking which I find essential for lowering my commit entropy. Including pytest will encourage developers to make larger commits which makes review difficult. Also, I can't think of a single major open-source project that runs pytest in pre-commit. I think there are good reasons for that.

Ok, I agree to your reasoning. I've removed it. Instead, I've added it to the tox cofiguration in pyproject.toml. Please review this, and see if I did it in a proper manner. I want to allow developers to type a short command (e.g. tox to run all tests, including generation of coverage reports and any other artifacts of a test run).

[maresb]

@tomers, I just started to review. Are you still making changes?

[maresb]

Marking as draft for now, just click "Ready for review" when you'd like me to take a look.

[tomers]

This PR is becoming pretty big, so I'll stop here, and submit my future changes in a separate PR

[maresb]

It's a lot more conventional to put tests/ alongside src/. I don't feel particularly strongly about this, but unless there's a particular reason it may be better in terms of standardization to move it there.

[tomers]

It's a lot more conventional to put tests/ alongside src/. I don't feel particularly strongly about this, but unless there's a particular reason it may be better in terms of standardization to move it there.

I think it would be better to have separate tests directory in each relevant code section, e.g. render engine tests under src/labelle/lib/render_engines/tests. This will prevent building of similar hierarchy under a global tests directory, which might break in the future, if things move along.

[maresb]

I think it would be better to have separate tests directory in each relevant code section, e.g. render engine tests under src/labelle/lib/render_engines/tests. This will prevent building of similar hierarchy under a global tests directory, which might break in the future, if things move along.

Usually there is a flat hierarchy under tests/. This might make it slightly harder to distribute code without tests, but there are some people who are vocally in favor of always including tests. So having the separate tests directories is a bit unconventional, but I don't see any practical issues, so let's keep it it.

[tomers]

I haven't looked in detail about why = None is necessary for the rendering init, but it stands out to me as odd. Probably we shouldn't need to weaken function signatures in order to make tests easier. Maybe add a kwargs to the parameterize?

In cli.py we explicitly pass these arguments, which might be None, thus we must support the case where the argument value is None, and handle it in the function body, instead of in the declaration.

[maresb]

This still seems fishy to me. Why is the value None in the first place? If they're coming from cli.py then perhaps our defaults there are incorrect. For example, I think this is wrong:

https://github.com/labelle-org/labelle/blob/d75a71e63fb2864d2105b93f926c1ef4bb9b46be/src/labelle/cli/cli.py#L214

Shouldn't it be this?

] = DEFAULT_BARCODE_TYPE

Also, I'm not sure if you saw this remark, or if it's hidden for you: https://github.com/labelle-org/labelle/pull/32#discussion_r1584773683

Thanks for all your work on this!

[tomers]

@maresb I've fixed the default handling of barcode type. If possible, please review this and merge. I got lost amongs the different PRs that we got. I'll leave it to you to merge. I guess I'll have some more breaking changes coming up soon. Would it be ok that you release a new major version with breaking changes, and then we introduce more breaking changes, or go you prefer that we keep work on main branch, without releasing, and then release once we're done?

[maresb]

@tomers, would you be able to address the remaining two default arguments? It's pretty minor, so please just merge yourself afterwards. (See the two "requested changes" which I just marked above as unresolved.)

Would it be ok that you release a new major version with breaking changes, and then we introduce more breaking changes, or go you prefer that we keep work on main branch, without releasing, and then release once we're done?

My wish would be to merge this PR, then #37, then #25, make one or more patch releases, and then start with the breaking changes. Would that work well with your plans?

[tomers]

@maresb, I am blocked from merging this PR. I guess it is because you're the reviewer. Please merge.

[maresb]

I am blocked from merging this PR.

That's annoying, sorry about that! I don't want that to happen.

Looking under https://github.com/labelle-org/labelle/settings/branch_protection_rules/49111150, I see

I think that's what's preventing you from merging as a maintainer. I'd prefer to keep this normally checked in order to avoid accidents, but please feel free to temporarily remove this protection in cases like this.

[maresb]

Unchecking that box gives maintainers the following override: