-
Notifications
You must be signed in to change notification settings - Fork 873
fix(tests): Minor fixes #3327
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
base: main
Are you sure you want to change the base?
fix(tests): Minor fixes #3327
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes CI issues by updating tests to use built-in pytest monkeypatching utilities and stabilizing a UI navigation step.
Changes:
- Replaced
pytest-mock(mocker) usage withmonkeypatchin Python unit tests. - Updated CUDA/installation-related tests to patch module/functions via
monkeypatch. - Adjusted Playwright navigation to wait for
domcontentloadedduring page load.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/unit/data/utils/test_image.py | Migrates image mask tests from mocker.patch to monkeypatch.setattr for CI compatibility. |
| tests/unit/cli/test_installation.py | Reworks installation utility tests to avoid pytest-mock and patch dependencies via monkeypatch. |
| application/ui/tests/main.spec.ts | Makes UI test navigation more deterministic by waiting for domcontentloaded. |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/anomalib/cli/utils/installation.py:123
- Relying on
len(requirement.specifier)being supported is potentially brittle acrosspackagingversions (and the intent here is just to detect multiple comma-separated constraints). Consider using a version-agnostic approach such as counting via iteration (e.g.,sum(1 for _ in requirement.specifier)) or checking for a comma instr(requirement.specifier)to avoid depending on__len__support.
if requirement.name.lower() == "torch":
torch_requirement = str(requirement)
if len(requirement.specifier) > 1:
warn(
"requirements.txt contains. Please remove other versions of torch from requirements.",
stacklevel=2,
)
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Test that get_requirements returns the expected dictionary of requirements.""" | ||
| requirements = get_requirements("anomalib") | ||
| assert isinstance(requirements, dict) |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test calls get_requirements(\"anomalib\") before stubbing installation.requires, which makes the first half of the test depend on the actual installed package metadata (can be absent or differ in CI). Make the test deterministic by monkeypatching installation.requires before the first call to get_requirements, returning a controlled list of requirement strings, and assert the resulting dict contents (extras/groups and parsed requirements), not just the types.
tests/unit/cli/test_installation.py
Outdated
| monkeypatch.setattr(installation, "requires", lambda _module="anomalib": None) | ||
| assert get_requirements() == {} | ||
|
|
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test calls get_requirements(\"anomalib\") before stubbing installation.requires, which makes the first half of the test depend on the actual installed package metadata (can be absent or differ in CI). Make the test deterministic by monkeypatching installation.requires before the first call to get_requirements, returning a controlled list of requirement strings, and assert the resulting dict contents (extras/groups and parsed requirements), not just the types.
| if version not in AVAILABLE_TORCH_VERSIONS: | ||
| version = max(AVAILABLE_TORCH_VERSIONS.keys()) | ||
| warn( |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max(AVAILABLE_TORCH_VERSIONS.keys()) compares version strings lexicographically, which can select the wrong 'latest' version (e.g., '2.10.0' sorts before '2.9.0'). Use a proper version comparison (e.g., packaging.version.Version) when selecting the maximum available Torch version.
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Requirement("torch==2.0.0"), | ||
| Requirement("onnx>=1.8.1"), | ||
| ] | ||
| torch_req, other_reqs = parse_requirements(requirements) |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is asserting get_requirements() == {} after patching get_cuda_version, but get_requirements doesnβt use get_cuda_version. Also, requires is still patched to return a non-empty list, so get_requirements() will continue returning requirements, making the final assertion incorrect/flaky. Patch anomalib.cli.utils.installation.requires to return None (or stop/reset the original patch) before asserting the empty dict.
| torch_req, other_reqs = parse_requirements(requirements) | |
| mocker.patch("anomalib.cli.utils.installation.get_cuda_version", return_value=None) | |
| # When no package name is provided and `requires` returns None, get_requirements should return an empty dict. | |
| mocker.patch("anomalib.cli.utils.installation.requires", return_value=None) |
| if len(requirement.specs) > 1: | ||
| if len(requirement.specifier) > 1: | ||
| warn( | ||
| "requirements.txt contains. Please remove other versions of torch from requirements.", |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning message is incomplete/unclear ("requirements.txt contains."), which makes it harder to action when it appears in CI logs. Consider rewording it to explicitly state what was detected (e.g., multiple torch specifiers found) and what to do next.
| "requirements.txt contains. Please remove other versions of torch from requirements.", | |
| f"Multiple version specifiers for 'torch' detected in requirements.txt " | |
| f"(requirement: '{requirement}'). Please specify only one torch version " | |
| "and remove any additional torch specifiers from your requirements.", |
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
π Description
β¨ Changes
Select what type of change your PR is:
β Checklist
Before you submit your pull request, please make sure you have completed the following steps:
For more information about code review checklists, see the Code Review Checklist.