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.

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

[pytest] Don't return values from test_* functions #14475

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

kparzysz-quic
Copy link
Contributor

Pytest expects test functions to return None, and errors to be flagged via exceptions. It actually emits warnings for "return" statements encountered in test_ functions.

Pytest expects test functions to return None, and errors to be flagged
via exceptions. It actually emits warnings for "return" statements
encountered in test_ functions.
@tvm-bot
Copy link
Collaborator

tvm-bot commented Apr 3, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: pytest See #10317 for details

Generated by tvm-bot

Copy link
Contributor

@Lunderberg Lunderberg left a comment

Choose a reason for hiding this comment

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

Thank you for the catch, and looks good to me! If I recall correctly, these were initially intended to reduce duplication between some of the tests and the test fixtures, as a workaround for pytest's lack of subtests, but I missed the return statements in pre-PR cleanup.

@@ -151,7 +151,7 @@ def test_maxpool(self, shape_nhwc, window_size, stride, pad, dtype, target):
padding=(pad, pad, pad, pad),
dtype=dtype,
)
return output, ref_output
assert all([output is not None, ref_output is not None])
Copy link
Contributor

Choose a reason for hiding this comment

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

If the assert is causing spurious unit test failures, it can be removed. The test is to ensure that no exceptions are through while lowering/building, and no further assert is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did remove it at first, but then the linter complained that output and ref_output were unused values... :|

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it. So long as the build_and_run function is called in the test, the return value should be ignoreable here.

@kparzysz-quic kparzysz-quic merged commit 579d999 into apache:main Apr 4, 2023
@kparzysz-quic kparzysz-quic deleted the no-return-in-test branch April 4, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants