Enhance two integration tests #511

robinwnv · 2022-08-05T08:30:34Z

Enhance test_append and test_array_creation

add negative tests
add more test cases
refactor test code

Enhance test_append and test_array_creation 1. add negative tests 2. add more test cases 3. refactor test code

for more information, see https://pre-commit.ci

tests/integration/test_append.py

bryevdv · 2022-08-05T16:38:19Z

tests/integration/test_append.py

+        num.append(a, c, axis=1)
+
+    with pytest.raises(IndexError):
+        num.append(a, a, axis=5)


Suggest splitting these up. Best practice is to have descriptively named tests that check a one thing. A class can be useful for grouping related test cases together:

class TestAppendErrors: def test_bad_dimension(self): # arrange msg = "All arguments to concatenate must have the same number of dimensions" with pytest.raises(ValueError, match=msg): num.append(a, b, axis=1) def test_bad_index(self): # arrange with pytest.raises(IndexError): num.append(a, a, axis=5)

The test output will look like TestAppendErrors:test_bad_dimension etc. and the class name can be conveniently utilized in test selection as well.

I'm also concerned about the AssertionError case. Assertions should be used to control internal program invariants, and generally speaking, should be not applied to user-supplied values. The reason being: we can only make assertions about things we control, but we have no control over what users may pass. So I would not expect to be maintenance of an AssertionError related to any value passed to a public API. This seems like a situation where the function should be raising a ValueError instead. That's what numpy does:

In [6]: np.append(a, c, axis=1) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) Input In [6], in <cell line: 1>() ----> 1 np.append(a, c, axis=1) File <__array_function__ internals>:180, in append(*args, **kwargs) File ~/anaconda3/lib/python3.8/site-packages/numpy/lib/function_base.py:5392, in append(arr, values, axis) 5390 values = ravel(values) 5391 axis = arr.ndim-1 -> 5392 return concatenate((arr, values), axis=axis) File <__array_function__ internals>:180, in concatenate(*args, **kwargs) ValueError: all the input array dimensions for the concatenation axis must match exactly, but along dimension 0, the array at index 0 has size 1 and the array at index 1 has size 10

@magnatelee can you comment?

@magnatelee It's found only in eager execution test, it raises ValueError. In other tests, it raises AssertionError.

tests/integration/test_array_creation.py

bryevdv · 2022-08-05T16:53:16Z

tests/integration/test_array_creation.py

+
+    num.full((2, 3), [1])
+    with pytest.raises(AssertionError):
+        num.full((2, 3), [10, 20, 30])


Suggest splitting these individual cases into individual tests, similar to the suggestion above. Then also the shape could be passed using pytest.mark.parametrize, which could be applied to the class:

@pytest.mark.parametrize('shape', shapes) class TestCreationErrors: def test_empty(self, shape): with pytest.raises(ValueError): num.empty(shape) def test_zeros(self, shape): with pytest.raises(ValueError): num.zeros(shape) ..... # additional special case for full def test_creation_full_assertion(): num.full((2, 3), [1]) with pytest.raises(AssertionError): num.full((2, 3), [10, 20, 30])

Though, my same concern above about AssertionError also applies here.

It's found only in eager execution test, it raises ValueError. In other tests, it raises AssertionError.

tests/integration/test_array_creation.py

tests/integration/utils/utils.py

1. Create test class for negative testing 2. Refactor out test functions 3. Use parameterize

for more information, see https://pre-commit.ci

tests/integration/test_array_creation.py

1. update run_test name to check_array_method 2. use parameterize for step zero cases of arange

robinwnv · 2022-08-10T09:52:26Z

tests/integration/test_array_creation.py

 class TestArrangeErrors:
    def test_negative_sizes(self):
        with pytest.raises(ValueError):
-            ###np.arange(-10) returns [] successfully
+            # np.arange(-10) returns [] successfully


@magnatelee Is this difference in results between numpy and cunumeric here expected or a bug?

note that in case there are currently expected failures, we can still have the test run (and avoid leaving commented code in the repo) by using @pytest.mark.xfail

I like this approach. If an xfail test switches to succeeding (when we fix the corresponding bug), will that show up as a test suite failure?

@manopapad yes it will which is a nice way to know in case you've accidentally fixed a bug :)

@bryevdv I like this approach, too. For AssertionError raised by cuNumeric, do we need to expect ValueError as NumPy in our test code and make it xfail?

do we need to expect ValueError as NumPy in our test code and make it xfail?

That is certainly an option although if this is a very simple fix I'd say just fix it (to raise ValueError) in this PR if possible.

I think xfail is definitely appropriate for the numerical discrepancies, since those probably should be fixed in a dedicated PR.

AssertionError is fixed. It raises ValueError now. Other failures are added with xfail.

robinwnv · 2022-08-10T09:54:15Z

tests/integration/test_array_creation.py

-    ### output: num: array([ 1, -1, -3, -5, -7]),
-    ### np: array([ 1. , -1.5, -4. , -6.5, -9. ]
+    # output: num: array([ 1, -1, -3, -5, -7]),
+    # np: array([ 1. , -1.5, -4. , -6.5, -9. ]
    # (1, -10, -2.5),


@magnatelee Is this difference in results between numpy and cunumeric here expected or a bug?

robinwnv · 2022-08-10T09:58:28Z

tests/integration/test_array_creation.py

@@ -76,24 +85,38 @@ def test_full(value):
 SHAPES_NEGATIVE = [
    -1,
    (-1, 2, 3),
-    # num.array([2, -3, 4]),  ## it would raise RuntimeError("Unable to find attachment to remove") when num.array
-    # is removed at the end as global variable
+    ## it raises RuntimeError("Unable to find attachment to remove")


@magnatelee Is this expected or an issue here? Thanks

manopapad · 2022-08-10T21:27:37Z

AFAICT all the differences that @robinw0928 has pointed out above are real bugs, including the cases where cunumeric asserts-out instead of throwing a ValueError like NumPy does. @robinw0928 can you open issues for these, so we can track work on fixing them? Alternatively, feel free to submit fixes within this PR if it's something small.

1. add pytest.mark.xfail for cases with expected failure 2. Small Fix: replace Assert with raising ValueError in deferred.py

for more information, see https://pre-commit.ci

bryevdv · 2022-08-12T16:08:11Z

LGTM!

manopapad · 2022-08-13T00:28:15Z

cunumeric/deferred.py

-                assert result.shape[dim] == 1
+                if result.shape[dim] != 1:
+                    raise ValueError(
+                        f"Shape did not match along dimension {dim}"


Leave a space at the end of the string here, otherwise when they get joined it will look like this: "... dimension 3and the ..."

Fixed. Thanks. @manopapad After this review, can I merge the change?

* Enhance two integration tests Enhance test_append and test_array_creation 1. add negative tests 2. add more test cases 3. refactor test code * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address comments 1. Create test class for negative testing 2. Refactor out test functions 3. Use parameterize * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address comments - part2 1. update run_test name to check_array_method 2. use parameterize for step zero cases of arange * Address comments - Part 3 1. add pytest.mark.xfail for cases with expected failure 2. Small Fix: replace Assert with raising ValueError in deferred.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Address comments - fix a typo Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Enhance two integration tests

a7783d2

Enhance test_append and test_array_creation 1. add negative tests 2. add more test cases 3. refactor test code

robinwnv requested review from magnatelee and bryevdv August 5, 2022 08:30

[pre-commit.ci] auto fixes from pre-commit.com hooks

7d99fb3

for more information, see https://pre-commit.ci

bryevdv reviewed Aug 5, 2022

View reviewed changes

robinwnv requested a review from marcinz August 8, 2022 03:01

robinwnv and others added 2 commits August 9, 2022 16:44

Address comments

8470c40

1. Create test class for negative testing 2. Refactor out test functions 3. Use parameterize

[pre-commit.ci] auto fixes from pre-commit.com hooks

5796cb9

for more information, see https://pre-commit.ci

bryevdv reviewed Aug 9, 2022

View reviewed changes

tests/integration/test_array_creation.py Outdated Show resolved Hide resolved

Address comments - part2

d3c2261

1. update run_test name to check_array_method 2. use parameterize for step zero cases of arange

robinwnv commented Aug 10, 2022

View reviewed changes

robinwnv and others added 2 commits August 12, 2022 16:07

Address comments - Part 3

edb9364

1. add pytest.mark.xfail for cases with expected failure 2. Small Fix: replace Assert with raising ValueError in deferred.py

[pre-commit.ci] auto fixes from pre-commit.com hooks

e045f8b

for more information, see https://pre-commit.ci

bryevdv approved these changes Aug 12, 2022

View reviewed changes

manopapad reviewed Aug 13, 2022

View reviewed changes

Address comments - fix a typo

02bc9da

marcinz approved these changes Aug 15, 2022

View reviewed changes

robinwnv merged commit 41a2f90 into nv-legate:branch-22.10 Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance two integration tests #511

Enhance two integration tests #511

robinwnv commented Aug 5, 2022

bryevdv Aug 5, 2022

robinwnv Aug 10, 2022

bryevdv Aug 5, 2022

robinwnv Aug 10, 2022

robinwnv Aug 10, 2022

bryevdv Aug 10, 2022

manopapad Aug 10, 2022

bryevdv Aug 10, 2022

robinwnv Aug 11, 2022

bryevdv Aug 11, 2022 •

edited

Loading

robinwnv Aug 12, 2022

robinwnv Aug 10, 2022

robinwnv Aug 10, 2022

manopapad commented Aug 10, 2022

bryevdv commented Aug 12, 2022

manopapad Aug 13, 2022

robinwnv Aug 15, 2022

Enhance two integration tests #511

Enhance two integration tests #511

Conversation

robinwnv commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryevdv Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manopapad commented Aug 10, 2022

bryevdv commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryevdv Aug 11, 2022 •

edited

Loading