Skip to content

Add a default value to CallInfo.result#3560

Merged
alanbato merged 2 commits into
pytest-dev:masterfrom
alanbato:fix_callinfo_rrp
Jun 10, 2018
Merged

Add a default value to CallInfo.result#3560
alanbato merged 2 commits into
pytest-dev:masterfrom
alanbato:fix_callinfo_rrp

Conversation

@alanbato

@alanbato alanbato commented Jun 10, 2018

Copy link
Copy Markdown
Member

Fixes #3554

Any comments/questions welcome :)

nicoddemus
nicoddemus approved these changes Jun 10, 2018
View reviewed changes

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great, thanks @alanbato!

I added some formatting to the CHANGELOG entry, after CI passes this is good to merge IMHO.

@coveralls

coveralls commented Jun 10, 2018 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.03%) to 92.732% when pulling 198e993 on alanbato:fix_callinfo_rrp into 7c8d072 on pytest-dev:master.

@alanbato alanbato merged commit 80f8a3a into pytest-dev:master Jun 10, 2018
@alanbato

alanbato commented Jun 10, 2018

Copy link
Copy Markdown
Member Author

Thanks @nicoddemus!

@alanbato alanbato deleted the fix_callinfo_rrp branch June 10, 2018 04:21
RonnyPfannschmidt
RonnyPfannschmidt reviewed Jun 10, 2018
View reviewed changes

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a breaking change, it has to be reverted

@nicoddemus

nicoddemus commented Jun 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Is CallInfo an external API? What breaks by this change?

@nicoddemus

nicoddemus commented Jun 10, 2018

Copy link
Copy Markdown
Member

@RonnyPfannschmidt is right, indeed it is used by:

  • pytest_runtest_makereport(item, call)
  • pytest_exception_interact(node, call, report)

Someone might be checking hasattr(call, 'result') to check if the call succeeded.

I will open a PR to revert this from master shortly. I think it is OK to move this to features though?

nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Jun 10, 2018
@nicoddemus
Revert change of Callinfo.result default value
10b0b81 
As discussed in pytest-dev#3560, this should not go to master because this breaks
the API.

Reverts commits:

1a7dcd7
198e993
@nicoddemus nicoddemus mentioned this pull request Jun 10, 2018
Merged
@nicoddemus

nicoddemus commented Jun 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Opened #3562, thanks @RonnyPfannschmidt for catching my slip up

@RonnyPfannschmidt

RonnyPfannschmidt commented Jun 10, 2018

Copy link
Copy Markdown
Member

AS per original issue callinfo needs a New state, something Like pre-call

@nicoddemus

nicoddemus commented Jun 10, 2018

Copy link
Copy Markdown
Member

AS per original issue callinfo needs a New state, something Like pre-call

I see, because result = None is a valid function return value. But you propose to update __repr__ to handle this "new state" then?

@RonnyPfannschmidt

RonnyPfannschmidt commented Jun 10, 2018

Copy link
Copy Markdown
Member

yes

@nicoddemus

nicoddemus commented Jun 10, 2018

Copy link
Copy Markdown
Member

Agreed. @alanbato want to tackle that?

@alanbato

alanbato commented Jun 11, 2018

Copy link
Copy Markdown
Member Author

Sure! Just to be sure though, do we want to include a new attribute that represents the different states? Something like CallInfo.is_finished. Or just do not has_attr(self, result) in the __repr__ func to print a different version of the repr string?

@RonnyPfannschmidt

RonnyPfannschmidt commented Jun 11, 2018

Copy link
Copy Markdown
Member

@alanbato i beleive a getattr(self, 'result', NOTSET) followed by an is check is helpful there

NOTSET is defined in compat.py, and im thinking switching to attr.NOTHING in the long term

@nicoddemus

nicoddemus commented Jun 11, 2018

Copy link
Copy Markdown
Member

@RonnyPfannschmidt sounds good. I think the same check must be done for excinfo, I think it is also set only when an exception occurs.

@RonnyPfannschmidt

RonnyPfannschmidt commented Jun 11, 2018

Copy link
Copy Markdown
Member

@nicoddemus that one is None at the class level

@nicoddemus

nicoddemus commented Jun 11, 2018

Copy link
Copy Markdown
Member

Oh OK, thanks

@blueyed

blueyed commented Aug 30, 2018

Copy link
Copy Markdown
Contributor

This was reverted in #3562 - what is the plan for it?

@RonnyPfannschmidt

RonnyPfannschmidt commented Aug 30, 2018

Copy link
Copy Markdown
Member

well, i outlined the requirements for a correct fix above

@blueyed blueyed mentioned this pull request Sep 11, 2018
Closed
@blueyed

blueyed commented Sep 11, 2018

Copy link
Copy Markdown
Contributor

@alanbato
Do you still plan to create a new PR?

blueyed added a commit to blueyed/pytest that referenced this pull request Nov 13, 2018
@blueyed
Fix CallInfo.__repr__ for unfinished call
8da6ecf 
Fixes pytest-dev#3554
Ref: pytest-dev#3560
Ref: pytest-dev#3562
@blueyed blueyed mentioned this pull request Nov 13, 2018
Merged
blueyed added a commit to blueyed/pytest that referenced this pull request Nov 13, 2018
@blueyed
Fix CallInfo.__repr__ for unfinished call
a8b011d 
Fixes pytest-dev#3554
Ref: pytest-dev#3560
Ref: pytest-dev#3562
blueyed added a commit to blueyed/pytest that referenced this pull request Nov 13, 2018
@blueyed
Fix CallInfo.__repr__ for unfinished call
27dab4e 
Fixes pytest-dev#3554
Ref: pytest-dev#3560
Ref: pytest-dev#3562
@blueyed

blueyed commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

New attempt in #4381.

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

Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt left review comments

@nicoddemus nicoddemus nicoddemus approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alanbato @coveralls @nicoddemus @RonnyPfannschmidt @blueyed