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

Fix issue where a new line was always written for the live log finish section #3194

Merged

Conversation

s0undt3ch
Copy link
Contributor

This is a fix for the feature PR #3189 where we'd always get 2 new lines between each log message on the finish section.

  • Target the features branch for new features and removals/deprecations.
  • Include new tests or update existing tests when applicable.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Minor comments on my part, thanks @s0undt3ch!

self.stream.write('\n')
self._first_record_emitted = True
elif self._when in ('teardown', 'finish'):
if self._test_outcome_written is False:
Copy link
Member

Choose a reason for hiding this comment

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

It is considered more Pythonic to write if not self._test_outcome_written instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But less explicit. Personal habits...

I'll change.

@@ -293,6 +294,62 @@ def test_log_2(fix):
])


def test_sections_single_new_line_after_test_outcome(testdir, request):
"""Check that with live logging enable we are printing the correct headers during
Copy link
Member

Choose a reason for hiding this comment

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

It seems this docstring is not related to the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, copy/paste

@nicoddemus nicoddemus requested a review from twmr February 8, 2018 23:31
@s0undt3ch s0undt3ch force-pushed the feature/logstart-logfinish branch from aa70cc4 to 0f4092e Compare February 8, 2018 23:35
@s0undt3ch
Copy link
Contributor Author

Updated.

@s0undt3ch s0undt3ch force-pushed the feature/logstart-logfinish branch from 0f4092e to 6491833 Compare February 8, 2018 23:36
@nicoddemus
Copy link
Member

I tested out the branch and it works as advertised. 👍

Thanks a lot @s0undt3ch!

@coveralls
Copy link

coveralls commented Feb 8, 2018

Coverage Status

Coverage increased (+0.005%) to 92.64% when pulling d776e56 on s0undt3ch:feature/logstart-logfinish into bba258a on pytest-dev:features.

@s0undt3ch s0undt3ch force-pushed the feature/logstart-logfinish branch from 6491833 to d776e56 Compare February 9, 2018 11:17
@nicoddemus nicoddemus merged commit 05faa69 into pytest-dev:features Feb 16, 2018
@s0undt3ch s0undt3ch deleted the feature/logstart-logfinish branch February 16, 2018 23:41
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