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

Add support for attaching logs on error. #34

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

Conversation

emonty
Copy link

@emonty emonty commented Jan 16, 2017

Sometimes, code being tested can produce a large amount of output which
is needed to debug issues. Adding that output to successful tests can
lead to blowing out size maximums for a full test suite run, even if
only one test fails, because of adding the log streaming output to every
test all the time.

To empower users to only collect that output on failures, expose a flag
on the constructor which causes addOnException to be used instead of the
normal addDetail. The default stays at off so existing users should not
see a behavior change.


This change is Reviewable

Sometimes, code being tested can produce a large amount of output which
is needed to debug issues. Adding that output to successful tests can
lead to blowing out size maximums for a full test suite run, even if
only one test fails, because of adding the log streaming output to every
test all the time.

To empower users to only collect that output on failures, expose a flag
on the constructor which causes addOnException to be used instead of the
normal addDetail. The default stays at off so existing users should not
see a behavior change.
Copy link
Member

@freeekanayaka freeekanayaka left a comment

Choose a reason for hiding this comment

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

There are no unit tests for the change (and the branch as it is actually breaks the existing ones).

More importantly, I don't quite understand what the intent of this branch is.


def _setUp(self):
write_stream, read_stream = self._stream_factory()
self.stream = write_stream
self._read_stream = read_stream
if self._only_on_error:
self.addOnException(self._add_stream_detail)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense to me. There's no "addOnException" method in the Fixture API.



def _byte_stream_factory():
result = io.BytesIO()
return (result, result)


def ByteStream(detail_name):
def ByteStream(detail_name, only_on_error=only_on_error):
Copy link
Member

Choose a reason for hiding this comment

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

There's no global variable called "only_on_error", so the Python interpreter tracebacks when getting to this line of code (see also the unit test failures attached to this branch by Travis).

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.

2 participants