Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Greptile SummaryThis PR completes the migration from Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[nose2 CLI\npython -m nose2 -A filter] --> B[AttributeGeneratorFilter.handleArgs\nnose2_attrib_generators.py]
B --> C{Generators plugin\nfound?}
C -- Yes --> D[Monkey-patch\n_testsFromGeneratorFunc]
C -- No --> E[Log warning\nno-op]
D --> F[Test Discovery]
F --> G{Is test a\ngenerator function?}
G -- Yes --> H[patched_testsFromGeneratorFunc\nevent, obj]
G -- No --> I[Normal nose2\ndiscovery]
H --> J{_matches_attrib_filter\nobj, attrib_plugin}
J -- No match --> K[Return empty list\nskip generator]
J -- Match --> L[Call original\n_testsFromGeneratorFunc]
L --> M[Yield parameterised\ntest cases]
I --> N[Run tests]
M --> N
K --> O[Test skipped]
Reviews (14): Last reviewed commit: "Review fix" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
dali/test/python/nose_utils.py, line 129-130 (link)style: creating a new
TestCaseinstance for each assertion call is unconventionalConsider creating a module-level instance once:
Then define
tcat module level (after imports) and reuse it in bothassert_raisesandassert_warnsNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
25 files reviewed, 1 comment
There was a problem hiding this comment.
Additional Comments (2)
-
dali/test/python/test_functional_api.py, line 22 (link)logic: This file still imports
nosedirectly and usesnose.tools.eq_on line 188. Since nose is being removed as a dependency, this will cause an import error. -
dali/test/python/test_functional_api.py, line 188 (link)logic: Replace
nose.tools.eq_with standard assertion or useassert_equalsfromnose_utilsNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
28 files reviewed, 2 comments
There was a problem hiding this comment.
Additional Comments (1)
-
dali/test/python/nose_utils.py, line 68 (link)syntax: missing
selfparameter - instance methods must haveselfas first parameter
29 files reviewed, 1 comment
|
!build |
|
CI MESSAGE: [40627092]: BUILD STARTED |
|
CI MESSAGE: [40627092]: BUILD FAILED |
|
!build |
|
CI MESSAGE: [40628704]: BUILD STARTED |
There was a problem hiding this comment.
Additional Comments (1)
-
dali/test/python/nose_utils.py, line 83-85 (link)style: Consider using the module-level
_test_caseinstance instead of creating a newempty_case()for each assertion:This is more efficient and aligns with how
assert_raisesandassert_warnsare implemented.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
29 files reviewed, 1 comment
|
Do we need to touch |
|
CI MESSAGE: [40628704]: BUILD FAILED |
mzient
left a comment
mzient
left a comment
There was a problem hiding this comment.
The way the tests are first discovered and then run will most certainly prevent parallel external source from working (see test job 247483858) was the primary reason for not pursuing full transition to nose2 to date.
Perhaps you can move the troublesome tests to separate files to guarantee proper execution order.
Also, some of the tests used to generate vast amounts of data in the "discovery" stage, because yielding in nose didn't cause data accumulation. Please make sure that this is not the case any more - the tests might work when run one by one but case out-of-memory condition when multiple test files are run.
There was a problem hiding this comment.
Additional Comments (1)
-
dali/test/python/nose_utils.py, line 74 (link)syntax: Missing
selfparameter in method definition
33 files reviewed, 1 comment
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
| expected_out, | ||
| ) | ||
|
|
||
| def test_numba_func_with_cond(self): |
There was a problem hiding this comment.
Missing @attr("sanitizer_skip") decorator
test_numba_func_with_cond is missing the @attr("sanitizer_skip") decorator that was present on the original standalone function. Without this attribute, the test will not be excluded when running with sanitizers enabled (via the -A '!sanitizer_skip' filter used in CI), causing potential test failures or false positives in sanitizer runs.
| def test_numba_func_with_cond(self): | |
| @attr("sanitizer_skip") | |
| def test_numba_func_with_cond(self): |
| expected_out=[np.full((10, 10, 10), 255, dtype=np.uint8)], | ||
| enable_conditionals=True, | ||
| ) | ||
| def test_numba_func_with_cond_do_not_convert(self): |
There was a problem hiding this comment.
Missing @attr("sanitizer_skip") decorator
test_numba_func_with_cond_do_not_convert is missing the @attr("sanitizer_skip") decorator that was present on the original standalone function. Without this attribute, the test will not be excluded when running with sanitizers enabled (via the -A '!sanitizer_skip' filter used in CI), causing potential test failures or false positives in sanitizer runs.
| def test_numba_func_with_cond_do_not_convert(self): | |
| @attr("sanitizer_skip") | |
| def test_numba_func_with_cond_do_not_convert(self): |
| def _build_attribs_list(self, attrib_plugin): | ||
| """Build the attribs list from the attrib plugin's -A configuration. | ||
|
|
||
| This replicates the logic from AttributeSelector.moduleLoadedSuite | ||
| for -A filters only (not -E eval filters). | ||
| """ | ||
| attribs = [] | ||
|
|
||
| # Handle -A (attribute) filters | ||
| for attr in attrib_plugin.attribs: | ||
| attr_group = [] | ||
| for attrib in attr.strip().split(","): | ||
| if not attrib: | ||
| continue | ||
| items = attrib.split("=", 1) | ||
| if len(items) > 1: | ||
| # "name=value" | ||
| key, value = items | ||
| else: | ||
| key = items[0] | ||
| if key[0] == "!": | ||
| # "!name" | ||
| key = key[1:] | ||
| value = False | ||
| else: | ||
| # "name" | ||
| value = True | ||
| attr_group.append((key, value)) | ||
| attribs.append(attr_group) | ||
|
|
||
| return attribs | ||
|
|
||
| def _matches_attrib_filter(self, test_func, attrib_plugin): | ||
| """Check if test_func matches the attribute filter from attrib plugin.""" | ||
| if not attrib_plugin: | ||
| return True | ||
|
|
There was a problem hiding this comment.
Duplicated internal attribute-parsing logic from nose2
_build_attribs_list manually replicates the parsing logic inside AttributeSelector.moduleLoadedSuite (including the comma-split, =-split, and !-negation handling). If nose2 changes how it parses -A expressions (e.g. to support quoting, ranges, or OR-groups within a single -A value), this copy will silently diverge and produce incorrect filter decisions for generator functions only.
Consider whether you can obtain the parsed attribs structure directly from the attrib_plugin instance after argument processing, rather than re-parsing from the raw string. For example, attrib_plugin.attribs may already be processed by the time handleArgs fires. If not, at minimum adding a comment explaining that this must be kept in sync with nose2's AttributeSelector internals would prevent silent future breakage.
There was a problem hiding this comment.
Added a comment explaining that.
|
!build |
|
CI MESSAGE: [45822574]: BUILD STARTED |
|
CI MESSAGE: [45743750]: BUILD PASSED |
|
CI MESSAGE: [45822574]: BUILD FAILED |
|
CI MESSAGE: [45822574]: BUILD PASSED |
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
|
!build |
|
CI MESSAGE: [45987009]: BUILD STARTED |
|
CI MESSAGE: [45987009]: BUILD FAILED |
|
CI MESSAGE: [45987009]: BUILD PASSED |
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
|
@greptileai please rereview and close adressed comments |
|
!build |
|
CI MESSAGE: [46987477]: BUILD STARTED |
|
CI MESSAGE: [46987477]: BUILD PASSED |
7 participants
Use nose2 as the only testing framework. Drop nose.
Category: Other
Description:
Remove the WARs used to keep nose alive.
nose2 supports the yield-style test discovery by default @attr has a different filtering syntax (
-A) and just checks for presence of truthy test_foo.attribute_name. A decorator uses this mechanism for backward compatibility.nose2 splits with_setup(setup, teardown) into two separate decorators, a backward compatible decorator is added.
nottest sets special attribute.
SkipTest from unittest is recommended to be used directly (with the same functionality).
Test scripts are adjusted with minimal changes to run through nose2. Followup cleanup can be used for renaming.
Replace unsupported -m regex by attributes
Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A