Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion noxfile.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def tests(session:nox.Session):

# package and test dependencies
session.install("-e", ".")
session.install("pytest")
session.install("pytest", "coverage")


session.run("pytest", "-q")
Expand Down
7 changes: 7 additions & 0 deletions LabGym/tests/pytest.ini → pytest.ini
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# pytest.ini
[pytest]
testpaths = tests

markers =
slow: marks tests as slow
integration: markts integration tests
gui: marks tests requiring wx

# Avoid two coverage warnings from detectron2's import of cv2.
# (see Notes.opencv-python-and-coverage-warnings.txt)
filterwarnings =
Expand Down
45 changes: 45 additions & 0 deletions tests/conftest.py
Copy link
Contributor

@ruck94301 ruck94301 Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
"""
/tests/conftest.py

Shared pytest fixtures for LabGym tests
"""

# Standard library imports
import sys

# Related third party imports
import pytest

@pytest.fixture(scope="session")
def wx_app():
from LabGym.ui.gui import wxutils
import wx
app = wx.App()
yield app


@pytest.fixture
def mock_argv(monkeypatch):
monkeypatch.setattr(sys, 'argv', ['LabGym'])


@pytest.fixture
def sample_greyscale_frame():
import numpy as np
import cv2
frame = np.full((100, 100), 200, dtype = np.uint8)
cv2.circle(frame, (50, 50), 15, 50, -1)
return frame

@pytest.fixture
def sample_video_frames():
import numpy as np
import cv2
frames = []

for i in range(60):
frame = np.full((100, 100), 200, dtype = np.uint8)
cv2.circle(frame, (20 + i % 60, 50), 15, 50, -1)
frames.append(frame)

return frames
File renamed without changes.
File renamed without changes.
2 changes: 1 addition & 1 deletion LabGym/tests/INIT.sh → tests/linting/INIT.sh
Copy link
Contributor

@ruck94301 ruck94301 Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ IS_VENV () { [ -n "$VIRTUAL_ENV+1" ]; } # works in sh, bash, and zsh

AWK () { awk "$@"; }

PYFILES=$(cd .. && echo *.py)
PYFILES=$(cd ../../LabGym && echo *.py)
# printf "%s: %s\n" "\$PYFILES" "$PYFILES"

return
Expand Down
2 changes: 1 addition & 1 deletion LabGym/tests/pylint.sh → tests/linting/pylint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

source INIT.sh

PYFILES=$(echo ../*.py ../mywx/*.py)
PYFILES=$(echo ../../LabGym/*.py ../../LabGym/mywx/*.py)

OP=pylint # default OP
while [ $# -gt 0 ]; do
Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.