8000 Make fail() available for soft_assertions() by RockBomber · Pull Request #58 · assertpy/assertpy · GitHub
[go: up one dir, main page]
Skip to content

Make fail() available for soft_assertions() #58

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
Closed
RockBomber wants to merge 1 commit into from
Closed

Make fail() available for soft_assertions() #58

RockBomber wants to merge 1 commit into from

Conversation

@RockBomber
Copy link
Contributor
@RockBomber RockBomber commented Dec 29, 2016

No description provided.

@coveralls
Copy link
coveralls commented Dec 29, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 99.433% when pulling 8ec46da on RockBomber:master into 4e8b057 on ActivisionGameScience:master.

@saturnboy
Copy link
Contributor
saturnboy commented Jan 4, 2017 •
edited
Loading

@RockBomber Thanks for the PR. I'm not sure fail() should be allowed in soft assertions. I guess I was thinking that if you call fail() in a test, then you really want the test to fail. Can you share a use case or a testing workflow where it makes sense to fail soft?
8000

@RockBomber
Copy link
Contributor Author
RockBomber commented Jan 4, 2017

Hello, @saturnboy !
Example of my case:

from assertpy import assert_that, fail, soft_assertions

def case_1(test_data_1):
    if test_data_1.field_1 == '1':
        assert_that(test_data_1.field_2).is_equal_to('1')
    elif test_data_1.field_1 == '2':
        assert_that(test_data_1.field_2).is_equal_to('2')
    else:
        fail('Unexpected value of field_1: %s' % test_data_1.field_1)

def case_2(test_data_2):
    if test_data_2.field_1 == '3':
        assert_that(test_data_2.field_2).is_equal_to('3')
    elif test_data_2.field_1 == '4':
        assert_that(test_data_2.field_2).is_equal_to('4')
    else:
        fail('Unexpected value of field_1: %s' % test_data_2.field_1)

def test_my_cases(test_data_1, test_data_2):
    with soft_assertions():
        case_1(test_data_1)
        case_2(test_data_2)

Of course, I can add first additional assertion like
assert_that(test_data_1.field_1).is_in('1','2')
But if I will need add another acceptable value of field_1, I must remember to add it to both places.

@saturnboy
Copy link
Contributor
saturnboy commented Jan 8, 2017 •
edited
Loading

@RockBomber It looks like you are testing dependent values (like an if-and-only-if in a math proof), for example: iff a=1, then b=2. The design of fail() in unit testing is to force a test failure, still not convinced it should warn when inside the soft assertions context.

I'd encourage you think of one branch as the default, like this:

def case_1(test_data_1):
    if test_data_1.field_1 == '1':
        assert_that(test_data_1.field_2).is_equal_to('1')
    else: # default case
        assert_that(test_data_1.field_1).is_equal_to('2')
        assert_that(test_data_1.field_2).is_equal_to('2')

@RockBomber
Copy link
Contributor Author
RockBomber commented Jan 8, 2017

Ok, My PR violates design of original fail().
I can suggest new function sfail() instead:

def sfail(msg=''):
    global _soft_ctx
    if _soft_ctx:
        global _soft_err
        out = 'Fail: %s!' % msg if msg else 'Fail!'
        _soft_err.append(out)
        return
    fail(msg)

saturnboy added a commit that referenced this pull request Jan 8, 2017
@saturnboy
force fail to always fail immediately, see #58
9df9ca3
@saturnboy
Copy link
Contributor
saturnboy commented Jan 8, 2017

@RockBomber found a bug in the contextmanager, so thanks for that. Added some tests (see ca90355) in test_soft.py, so you can see the exact intended behavior of fail() in assertpy. Closing this PR.

Please open a new PR for soft_fail() if you still want that functionality. Please include some tests.

@saturnboy saturnboy closed this Jan 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@RockBomber @coveralls @saturnboy
0