Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add mock tests to ensure cupy.cuda.cub is used #3467

Merged
merged 8 commits into from Jun 29, 2020
Merged

Add mock tests to ensure cupy.cuda.cub is used #3467

merged 8 commits into from Jun 29, 2020

Conversation

leofang
Copy link
Member

@leofang leofang commented Jun 20, 2020

Follow up of #2598 and #3428. See #3428 (comment).

cc: @emcastillo

@leofang leofang changed the title [WIP] Add mock tests to ensure cupy.cuda.cub is used Add mock tests to ensure cupy.cuda.cub is used Jun 21, 2020
@leofang leofang marked this pull request as ready for review June 21, 2020 03:12
@kmaehashi kmaehashi added this to the v8.0.0b4 milestone Jun 22, 2020
@leofang
simplify formatting a bit
ae77eee
@leofang
further simplification
fa9768d 
It seems if the callee is a Python def function (not cdef/cpdef), then
mock has no problem in intercepting its return value, and we don't not
need to invent a side_effect for it.
@leofang
Copy link
Member Author

leofang commented Jun 23, 2020

Caught something unexpected. Some assert_called*() methods always evaluate to True but they shouldn't. Digging why...

@leofang
Copy link
Member Author

leofang commented Jun 23, 2020

I fixed it. The gotcha is using side_effect in mock.patch() would shadow too many things and make the asserts behave abnormally. PTAL.

@asi1024 asi1024 modified the milestones: v8.0.0b4, v8.0.0b5 Jun 24, 2020
@emcastillo emcastillo modified the milestone: v8.0.0b5 Jun 25, 2020
@emcastillo
Copy link
Member

The PR is awesome,
just left a minor comment to clean it up a bit.

@leofang
Copy link
Member Author

leofang commented Jun 25, 2020

Nice suggestion, Emilio, I cut more lines! PTAL.

emcastillo
emcastillo reviewed Jun 26, 2020
View reviewed changes
cupy/testing/helper.py Outdated Show resolved Hide resolved
leofang
leofang commented Jun 26, 2020
View reviewed changes
cupy/testing/__init__.py Outdated Show resolved Hide resolved
cupy/testing/helper.py Outdated Show resolved Hide resolved
tests/cupy_tests/core_tests/test_ndarray_reduction.py Outdated Show resolved Hide resolved
tests/cupy_tests/core_tests/test_ndarray_reduction.py Outdated Show resolved Hide resolved
tests/cupy_tests/math_tests/test_sumprod.py Outdated Show resolved Hide resolved
tests/cupy_tests/math_tests/test_sumprod.py Outdated Show resolved Hide resolved
tests/cupy_tests/math_tests/test_sumprod.py Outdated Show resolved Hide resolved
tests/cupy_tests/sorting_tests/test_search.py Outdated Show resolved Hide resolved
tests/cupy_tests/sorting_tests/test_search.py Outdated Show resolved Hide resolved
tests/cupyx_tests/scipy_tests/sparse_tests/test_csr.py Outdated Show resolved Hide resolved
@leofang
Copy link
Member Author

leofang commented Jun 26, 2020

Sounds good. Done!

@emcastillo
Copy link
Member

Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit c67c656:

@emcastillo emcastillo added the cat:test Test code / CI label Jun 29, 2020
@emcastillo emcastillo added the st:test-and-merge (deprecated) Ready to merge after test pass. label Jun 29, 2020
@chainer-ci
Copy link
Member

Jenkins CI test (for commit c67c656, target branch master) succeeded!

@mergify mergify bot merged commit 618d5b7 into cupy:master Jun 29, 2020
@leofang leofang deleted the cub_mock branch June 29, 2020 02:16
@leofang
Copy link
Member Author

leofang commented Jun 29, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emcastillo emcastillo emcastillo left review comments

Assignees

@emcastillo emcastillo

Labels
cat:test Test code / CI st:test-and-merge (deprecated) Ready to merge after test pass.
Projects
None yet
Milestone
v8.0.0b5
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@leofang @emcastillo @pfn-ci-bot @chainer-ci @kmaehashi @asi1024