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

Shed diff report #305

Merged
merged 7 commits into from Sep 25, 2015

Conversation

Projects
None yet
2 participants
@erasche
Copy link
Member

erasche commented Sep 19, 2015

This produces a (fake) XUnit report for the shed_diff command. It's not complete as-is, but it'll provide essential functionality immediately. The XUnit reports will tell you if a shed diff returned a repository as being identical, different (failure), or if the diff command failed (error). It will not show a copy of the diff as I couldn't figure out how to cleanly extract that.

@jmchilton if you have suggestions for how this should be better implemented, please let me know. I see that most of the commands are pretty small and most of that functionality is tucked away in planemo.shed and friends, but here I went with the implementation I was comfortable with. If you have suggestions how I should get at the diff output I'd be happy to make another PR with those (or update this one).

(I branched off of that branch, I didn't want to deal with merge issues since these both needed common changes to the code, blargh.)

erasche added some commits Sep 19, 2015

Reorganise to clean up environment
By using the to_json filter in jinja, we don't need
to manually render to a json string
@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Sep 19, 2015

Ping @martenson, this will make the shed tests much easier to read. :)

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Sep 19, 2015

Will get failing test cases figured out presently.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented on 9813ef0 Sep 19, 2015

Looks good to me! You are right that I would over-engineer a little more because that is what I do, but this looks perfectly fine to me 😄.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Sep 19, 2015

Let me know if you need help with the test cases, otherwise this looks very good to me. Thanks @erasche.

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Sep 19, 2015

@jmchilton got my issue figured out :) Also adding test cases for new shed_diff xunit report. Will push shortly.

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Sep 19, 2015

Okay, I'm at a loss for why my tests are still failing when they aren't locally. I'll attack this some more tomorrow.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Sep 19, 2015

Yeah, they work locally for me too. Pushed a small tweak to the test cases that should improve the assertion error message reporting.

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Sep 25, 2015

(TIL you can run individual tox tests with tox -e py27 -- -s --tests tests.test_shed_diff. Also will squash before this gets merged due to unnecessary number of commits because of Travis)

erasche added some commits Sep 19, 2015

Fix travis tests by sorting directories
@jmchilton apparently os.walk is nondeterministic. I believe This was
causing the repositories to be discovered in varying orders on Travis's
machines (as opposed to ours), and thus when the XML results were
compared to the static files, cat2 would be before cat1, *sometimes*.

From https://docs.python.org/2/library/os.html#os.walk

> impose a specific order of visiting

found via http://stackoverflow.com/a/9062668

@erasche erasche force-pushed the shed-diff-report branch from 220b2c1 to 452654c Sep 25, 2015

@erasche

This comment has been minimized.

Copy link
Member Author

erasche commented Sep 25, 2015

It finally passes! Whuuuu! @jmchilton haha. Sorry, just been wrestling with this for days and to find out it was all because of stupid non-deterministic library methods...

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Sep 25, 2015

Brilliant, thanks a bunch @erasche.

jmchilton added a commit that referenced this pull request Sep 25, 2015

@jmchilton jmchilton merged commit 65de593 into master Sep 25, 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 shed-diff-report branch Sep 25, 2015

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