Merge lp://staging/~gz/juju-release-tools/test_apply_patches into lp://staging/juju-release-tools

Proposed by Martin Packman
Status: Merged
Approved by: Martin Packman
Approved revision: 324
Merged at revision: 322
Proposed branch: lp://staging/~gz/juju-release-tools/test_apply_patches
Merge into: lp://staging/juju-release-tools
Prerequisite: lp://staging/~gz/juju-release-tools/utils_no_mock
Diff against target: 203 lines (+171/-5)
2 files modified
apply_patches.py (+11/-5)
tests/test_apply_patches.py (+160/-0)
To merge this branch: bzr merge lp://staging/~gz/juju-release-tools/test_apply_patches
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+300776@code.staging.launchpad.net

This proposal supersedes a proposal from 2016-07-21.

Commit message

Add test coverage for apply_patches script

Description of the change

Somehow I forgot on Friday that we do actually have test infrastructure for juju-release-tools, so this branch adds coverage. There are some fiddly bits (`with open` is always annoying), and one relevent change with the addition of flushing for the parent process's output. This ensures the ordering of output is correct when mixed with the patch command, which currently is a confusion on the logs.

The addition of gettext for the user informational output is total vanity. Please forgive.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Do we need print_now? I have strived to move away from it. I introduced it because the buffered stdout was not appearing near the output of other procs. Per you own example, I log.XXX() which is unbuffered stderr. I print() to stdout for user info. Do we have a buffering problem that this solves?

Removing it might be awkward because I can see a few tests are mocking it.

review: Needs Information (code)
Revision history for this message
Martin Packman (gz) wrote :

I agree print_now is ugly and generally the wrong thing.

The buffering issue this fixes can be seen in the log output at tarball creation at present:

    patching file gopkg.in/mgo.v2/session.go
    Applying 1 patches
    Applied patch '001-mgo.v2-issue-277-fix.diff'

The first line is from `patch` when it exits, but it should really appear between the two lines from the parent script for clarity. As stdout is buffered, those are only output when the parent process exits.

So, four possible fixes:

# Write to stderr not stdout
  - This is often the right thing for scripts. Use stdout for results - things that could actually be piped into another command - and use stderr for feedback to the user.
# Use logging module
  - Uses stderr like above, needs extra configuration but can be easier to reuse.
# Use print(flush=True)
  - Python 3.3 and above only
# Use a helper that calls .flush()

So, why did I not write to stderr... well, this is the kind of script I don't expect to have output beyond the statement of activity, so I've tended to put that on stdout. This is much like the patch command itself, which uses stdout. But, lets try the other way.

323. By Martin Packman

Switch to using stderr for apply_patches output

Revision history for this message
Curtis Hovey (sinzui) wrote :

I accept print_now as a compromise for now.

> # Write to stderr not stdout
> - This is often the right thing for scripts. Use stdout for results - things that could
> actually be piped into another command - and use stderr for feedback to the user.

Your example is a little different from my recent experience where I did want my
output captured by other scripts.

> # Use logging module
> - Uses stderr like above, needs extra configuration but can be easier to reuse.

I considered this for my recent cases, We still configure a logger without timestamps,

> # Use print(flush=True)
> - Python 3.3 and above only

Been there. I think we can switch when we stop supporting precise or we stop using this
set of scripts on precise

> # Use a helper that calls .flush()

Yep. the helper is the cleanest path for now.

review: Approve (code)
324. By Martin Packman

Remove mostly redundant test for apply_patches

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you for choosing a solution.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches