testing: reorg of test infra for maintainability and scalability#342
testing: reorg of test infra for maintainability and scalability#342yujiahu415 merged 4 commits intoumyelab:masterfrom
Conversation
|
Guys,
This PR would benefit from some discussion.
So far, I'm the only one producing unit tests.
You would want to give me an opportunity to comment before reorganizing the
unit tests.
16 hours from PR to merge without asking my input sends a strange message
to me.
This is not an urgent bugfix that needs to be included in this week's
release.
I recommend revert the merge of this PR for now, and continue discussion.
…On Thu, Jan 22, 2026 at 10:47 PM Yujia Hu ***@***.***> wrote:
Merged #342 <#342> into master.
—
Reply to this email directly, view it on GitHub
<#342 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4CQNDU5QVFVUJCDOES3RD4IGKUBAVCNFSM6AAAAACSSYSSP6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMRSGIZTENBQGYZDGMQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
I’m really sorry, John. I didn’t realize this. Sure, I’ll revert this PR for your inputs. And I’ll leave more time for possible comments and discussion for a PR before merging it in the future. See PR #343 |
|
Does I recently added a feature where LabGym could run its own unit tests. |
|
Looking at noxfile... Instead of adding "coverage" to noxfile, should we eliminate "pytest" from noxfile? I don't understand adding "coverage" to noxfile. There's no nox-driven testing that employs it, is there? But, now that we're looking at it, I'm thinking maybe the nox- install of pytest is no longer appropriate. pytest is in the pyproject.toml, now, so I suspect it is redundant to have nox install it separately. Instead of adding "coverage", I propose removing pytest from noxfile installation. More broadly, I recommend do not add "coverage" or other developer-only tools without more justification/consensus. There are developers tools that don't need to be in the project, but that individual developers might use. I'd love to persuade you that coverage analysis is useful and challenge you to expect more of contributors unit tests, but unless and until that's an actual expectation of contributors, I'd argue that developers personal environments -- including mine, of course -- don't belong in the pyproject.toml or noxfile. We aren't putting pylint in there, we aren't putting mypy in there, we aren't putting mypy stub-files in there. BTW, I don't install "coverage" separately/independently like that in my devtools venv, I install a package that has coverage as a prerequisite so pip gets it as part of something else. |
There was a problem hiding this comment.
Can this line be simply eliminated? I think the nox install of pytest was previously necessary, because pytest wasn't in pyproject.toml. But now it is. And nothing depends on "coverage", right?
There was a problem hiding this comment.
I like using the "markers" feature of pytest. I have some work (not yet PR-ed) where I employ one new marker "wxapp" to mark the tests that require the wx.App object, and I have a use-case for running all unit tests except those that require wx.App object.
I don't have a use case for slow and integration... As I continue to review, I'll look for how you are using them. If you are not using them then I will advise: don't organize for what doesn't have a use yet. The complexity is a type of "cost" that needs to have a reason to absorb it. We all, myself included, need to resist the impulse to create framework that is anticipatory.
There was a problem hiding this comment.
I like the use of conftest.py for shared pytest fixtures. I use that in a branch that I have not yet PR-ed.
But I'm looking at this and a little puzzled... your wx_app fixture relies on LabGym.ui.gui wxutils.
That doesn't exist right? Does the fixture work?
I don't see it being used, so I'm not sure what to make of it.
I'll look carefully for it and for your use of the mock_argv, sample_grayscale_frame, and sample_video_frames as I continue review.
Followup:
I do not see your fixtures being used and I'm thinking your wx_app is not viable.
One path forward is for me to PR my conftest.py, and sometime in future you add your fixtures when you get to the point you'd use them. I will probably roll exitstatus.py into a shared fixture as well, and eliminate that separate file.
There was a problem hiding this comment.
I'm prepared for this file to be eliminated, it falls into the category of personal-developer-tooling and shouldn't clutter the project. At one point I thought it would be more generally useful, but, that's looking unlikely.
Also, better to eliminate this directory and its contents. No new "linting" directory. And further, eliminate tmp.pylint/.gitkeep. All of this is very useful to me, but not GENERALLY useful, so it does not belong in project.
Either exclude from your PR and ask me to separately PR their removal, or remove them in your PR.
There was a problem hiding this comment.
I am uncomfortable with the (excessive?) hierarchicalization. Independent from the issue of the location of tests directory (which I will comment on in a moment),
the EXTRA separation "unit" vs. "notes" vs. "integration" isn't justified. And I've never seen it, TBH... is this something you've seen in open-source projects?
I strongly recommend that we all prefer "flat" until flat becomes unwieldy. There's lots of runway before that might happen and probably it never will. Keep it simple and flat.
|
Now, about the motivation for the movement... locating the tests at package-level vs at project-level.
I agree that project-level tests folder is an accepted good practice, but, to be clear, so is package-level tests folder.
Two responses...
That's an approach, but not the only approach. I'm still not yet actually seeing it as a potential cause of confusion. If you say that it genuinely confused you personally, I'll concede it as possibility, as my imagination is sometimes not strong enough. But if you're just anticipating confusion in others, I'd say let things evolve as needed.
Yes there are two tests folders. I'm getting worn out addressing things point-by-point.
I'm thinking even better to eliminate these scripts that are not justified. |
|
Now, about three widely-accepted organizations of code and tests:
Any of the above three work fine for LabGym. Pasting from Gemini: The src layout ... separates importable source code from other project assets. Key Benefits: In a flat layout, the package directory sits directly in the project root. Key Benefits: Location and Naming of Pytest Unit Tests
Structure: The Good |
|
I'm finally back from my significant travel fiasco - appreciate the feedback. Definitely some things in my original PR that I'd now like to address by changing in my code (likely via a new mini-PR, rather than a complete reversion of #342 ), and some things I'd like to comment on as well. Will have more tomorrow. Thanks |
Summary
Restructures the test directory layout in a manner that follows Python packaging best practices, and prepares the codebase for future test expansion.
Why this change?
Problem: tests were located inside the 'LabGym/' application package, which meant:
Solution: Move tests to a dedicated 'tests/' folder at the repository root, following a standard Python project layout used by major packages.
What Changed
Before:
After:
I moved some of John's notes into a designated notes folder, and I also aggregated his linting scripts into a 'linting' folder within '/tests/linting/'. I also edited John's linting scripts to reflect the new structure. Feel free to change the scripts if they didn't carry over as expected.
All 59 tests pass on my end via 'pytest -q'.