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

RFC: Use AssertionError when inside decorated test case #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kazk
Copy link
Member

@kazk kazk commented Jul 13, 2020

  • Stops at first failure like most other test frameworks
  • Single test passed message regardless of the number of assertions
  • Discourage fat test cases
  • Allow using external assertions
    • np.testing.assert_equal
    • pd.testing.assert_frame_equal
    • assertion packages

Tests written in old style are not affected.

Need some review and feedback.


Closes #9

kazk added 2 commits July 12, 2020 17:41
- Stops at first failure like most other test frameworks
- Single test passed message regardless of the number of assertions
- Discourage fat test cases
- Allow using external assertions
  - `np.testing.assert_equal`
  - `pd.testing.assert_frame_equal`
  - assertion packages
try:
func()
if is_test_case:
display("PASSED", "Test Passed")
Copy link
Member Author

Choose a reason for hiding this comment

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

This will show extra <PASSED::> if any assertion functions were used inside some wrapper.

@kazk kazk changed the title Use AssertionError when inside decorated test case RFC: Use AssertionError when inside decorated test case Jul 13, 2020
@ggorlen
Copy link
Contributor

ggorlen commented Aug 6, 2020

Need some review and feedback

Are you interested in feedback on the design, code/style, both, or some other aspect(s)?

@kazk
Copy link
Member Author

kazk commented Aug 6, 2020

Any feedback. But I'm honestly starting to think it's better to spend time on figuring out how to allow kata authors to choose unittest instead.

@ggorlen
Copy link
Contributor

ggorlen commented Sep 20, 2020

I'm curious what the rationale is behind limiting the AssertionError catch to the decorated test case function only. Yeah, unittest support would be great (if possible)!

@kazk
Copy link
Member Author

kazk commented Sep 21, 2020

I'm curious what the rationale is behind limiting the AssertionError catch to the decorated test case function only.

I'm not sure what you mean by catch. Can you post a link to the line or leave a comment on the line?


So old tests not using decorators looks like this:

test.it("old test")
test.assert_equals(1, 2)

If this assert_equals raised AssertionError, we won't get any failure message (i.e., no <FAILED::>) and test just crashes there.

@ggorlen
Copy link
Contributor

ggorlen commented Sep 21, 2020

Maybe "handle" is a better term. No particular line--the design seems based on # TODO Currently this only works if assertion functions are written directly in the test case., but wouldn't it be possible to try-except the entire decorated test function execution, catch (or except) any AssertionErrors that are raised (regardless of whether they were raised in the test case function or not) and print the necessary <FAILED::> along with whatever error message rode along the error object?

@kazk
Copy link
Member Author

kazk commented Sep 21, 2020

That comment means

import codewars_test as test
@test.describe("group 1")
def group_1():
    @test.it("test 1")
    def test_1():
        test.assert_equals(1, 2)

works, but

import codewars_test as test

def wrapped_equal(a, b):
    test.assert_equal(a, b)

@test.describe("group 1")
def group_1():
    @test.it("test 1")
    def test_1():
        wrapped_equal(1, 2)

doesn't because of the way it currently checks if it's inside a test case. We can make it work better, but I didn't think it's worth it for RFC.

wouldn't it be possible to try-except the entire decorated test function execution

We can do that if we didn't have to worry about the existing tests using assertions from this package (test.assert_equal) on top level. We can't change them to always throw AssertionError because that breaks a lot. And if it doesn't throw, we can't report the failure. That's why changes in this PR only changes their behavior when inside a test case.

@kazk
Copy link
Member Author

kazk commented Sep 21, 2020

Also, to be clear, that limitation only applies for assertions from this package. Anything that throws AssertionError should work even if it's deeply nested.

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.

Tests with AssertionError doesn't display PASSED
2 participants