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

Capture stdout/stderr for shed_diff and shed_update XUnit reports #344

Merged
merged 7 commits into from Oct 23, 2015

Conversation

Projects
None yet
3 participants
@erasche
Copy link
Member

erasche commented Oct 22, 2015

@jmchilton would you mind putting eyes to this if you have time? @martenson you might be interested in this as well.

The Good

<?xml version="1.0" encoding="UTF-8"?>
<testsuite name="planemo-update"
           tests="1"
           errors="0"
           failures="1"
           skip="0">
    <testcase classname="seqtk" name="" time="6.95435190201">
            <error type="planemo.FailedUpdate" message="Failed to update repository">
            </error>
        <system-out>
        cd '/home/hxr/work/tools-iuc/tools/seqtk' &amp;&amp; git rev-parse HEAD
        cd '/home/hxr/work/tools-iuc/tools/seqtk' &amp;&amp; git diff --quiet
        </system-out>
        <system-err>
        Could not update seqtk
        Unexpected response from galaxy: 400: {"content_alert": "", "err_msg": "No changes to repository."}
        </system-err>
    </testcase>
</testsuite>

XUnit reports for shed_update and shed_diff will now contain stdout/stderr, and make those available to browse in Jenkins

The Bad

Logs under --report_xunit are no longer pretty colours. They're boring black and white. Seems like an acceptable loss since you gain awesome XUnit reports.

If --report_xunit is not specified, tests are still colourful.

The Ugly

This PR provides a small class called planemo.io.Capturing which lets you capture stdout/stderr for any function calls, and then process that data at your convenience. You don't have to rearchitect any of the existing infrastructure for logging user facing messages, you can just capture it and deal with it as you please. This code smells and it's pretty stinky. Instead of some way to handle message propagation properly and dispatching messages to handlers which care about them, we monkey patch sys.std*. I am not proud of this, but it solves the problem simply and quickly with very few changes.

The whole of cmd_shed_update and cmd_shed_diff are probably in dire need of a refactoring

@erasche erasche added the enhancement label Oct 22, 2015

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Oct 22, 2015

(Rebasing + fixing tests now...)

@erasche erasche force-pushed the xunit-capture branch from a80fb61 to fecfb9e Oct 22, 2015

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Oct 22, 2015

Timestamps gone crazy, github is not displaying this well for me, but I tried refactoring a lot of the duplicated functionality out - hope this okay @erasche. Another upshot is that less of planemo.io needs to be exposed as well. Not sure it actually works yet though :).

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Oct 22, 2015

@jmchilton oh wow, that is absolutely ok. I definitely should've seen that easy place to refactor. I'll give it a test locally.

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Oct 23, 2015

@martenson did you have opinions on this?

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Oct 23, 2015

I am not experienced with stream handling so I cannot comment on the architecture but I like the functionality and simple approach. Thanks @erasche
👍

jmchilton added a commit that referenced this pull request Oct 23, 2015

Merge pull request #344 from galaxyproject/xunit-capture
Capture stdout/stderr for shed_diff and shed_update XUnit reports

@jmchilton jmchilton merged commit c5fdc3a into master Oct 23, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jmchilton jmchilton deleted the xunit-capture branch Oct 23, 2015

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Oct 23, 2015

This is fine for now, the approach needs to be reworked to be thread safe someday (i.e. log the output instead of capturing it) if we want to build a planemo web app. Thanks a bunch @erasche.

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Oct 23, 2015

Agreed. I just didn't have the time/energies for a full rewrite of all echoing to logging. Will have to contribute that someday.

A planemo web app? Are we building our own IDE? Neato.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Oct 23, 2015

Less of an IDE than just a shed management app: #169 (comment)

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