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

Smart shed_diff. #167

Merged
merged 2 commits into from May 1, 2015

Conversation

Projects
None yet
2 participants
@jmchilton
Copy link
Member

jmchilton commented May 1, 2015

  • Fix shed_diff so its exit code actually is non-zero if there are differences.
  • By default - don't report differences for tool shed populates attributes (toolshed, changeset_revision). This can be disabled with --raw.
  • Tests for ensure --raw and smart differencing are working and exit codes are fixed.
  • Update shed test webapp to inject the tool shed populated fields into respective arguments.

Implements #141.

Shed-aware shed_diff...
 - Fix shed_diff so its exit code actually is non-zero if there are differences.
 - By default - don't report differences for tool shed populates fields (toolshed, changeset_revision). This can be disabled with --raw.
 - Tests for ensure --raw and smart differencing are working and exit codes are fixed.
 - Update shed_app test app to inject the tool shed populated fields into respective arguments.
@erasche

This comment has been minimized.

Copy link
Member

erasche commented May 1, 2015

Fix shed_diff so its exit code actually is non-zero if there are differences.

+1

@erasche

This comment has been minimized.

Copy link
Member

erasche commented May 1, 2015

Update shed test webapp to inject the tool shed populated fields into respective arguments.

can you just subprocess.check_command(['sh', 'run_shed.sh', '--daemon'])?

@@ -29,6 +29,12 @@
" this to testtoolshed.",
default=None,
)
@click.option(
"--raw",

This comment has been minimized.

@erasche

erasche May 1, 2015

Member

unused?

This comment has been minimized.

@jmchilton

jmchilton May 1, 2015

Author Member

It is used inside of planemo.shed.

This comment has been minimized.

@erasche

erasche May 1, 2015

Member

Whoops. Missed that. Sorry!

@@ -53,7 +59,7 @@ def cli(ctx, path, **kwds):
def diff(realized_repository):
working = tempfile.mkdtemp(prefix="tool_shed_diff_")
try:
shed.diff_in(ctx, working, realized_repository, **kwds)
return shed.diff_in(ctx, working, realized_repository, **kwds)

This comment has been minimized.

@erasche

erasche May 1, 2015

Member

only return in one place, returning (I'm assuming) an error code, or None. Maybe better to explicitly return None/-1? Sorry, it's late, tipsy code review is probably not the best ;)

This comment has been minimized.

@jmchilton

jmchilton May 1, 2015

Author Member

It is the last statement in the diff - it is wrapped in a finally not exception handling so this return is the only place to return from.

xml_b = ElementTree.parse(file_b).getroot()
_strip_shed_attributes(xml_a)
_strip_shed_attributes(xml_b)
return diff.diff(xml_a, xml_b, reporter=f.write)

This comment has been minimized.

@erasche
if not text_compare(x1.tail, x2.tail):
reporter('tail: %r != %r' % (x1.tail, x2.tail))
return False
return _compare_children(x1, x2, reporter)

This comment has been minimized.

@erasche

erasche May 1, 2015

Member

yep, used this too. Good choice.

@@ -0,0 +1,17 @@
<?xml version="1.0"?>
<repositories description="A suite of Galaxy tools designed to work with version 1.2 of the SAMtools package.">
<repository changeset_revision="cf875cbe2df4" name="data_manager_sam_fasta_index_builder" owner="devteam" toolshed="https://toolshed.g2.bx.psu.edu" />

This comment has been minimized.

@erasche

erasche May 1, 2015

Member

maybe test some without hardcoded changeset_revision? Not strictly necessary, just being picky since I'm a bottle and a half in :P

This comment has been minimized.

@jmchilton

jmchilton May 1, 2015

Author Member

Well I was a bottle and half in when I wrote this so seems appropriate :). This is meant to emulate the shed version - the plain version is in tests/repository_dependencies.xml - I use this to test that shed_diff filters the required attributes to make sure these files are actually different.

Thanks for the detailed review!



def _check(reporter):
assert not diff(ElementTree.fromstring("<moo>cow</moo>"),

This comment has been minimized.

@erasche
@erasche

This comment has been minimized.

Copy link
Member

erasche commented May 1, 2015

+1

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented May 1, 2015

can you just subprocess.check_command(['sh', 'run_shed.sh', '--daemon'])?

That would really increase the runtime of those tests - it takes well less than a second to start the server and run those tests. I also like mocking out the tool shed interface because it really crystalizes exactly the API planemo is consuming.

There is one functional test that actually exercises a real tool shed (https://github.com/galaxyproject/planemo/blob/master/tests/test_shed.py) and certainly it would be cool if that would automatically start a tool shed and stuff but I have not gotten around to it yet. It is a little bit harder than just run_tests.sh - since it should configure an isolated environment for the test.

jmchilton added a commit that referenced this pull request May 1, 2015

@jmchilton jmchilton merged commit 01ddcc2 into galaxyproject:master May 1, 2015

1 check passed

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

@jmchilton jmchilton referenced this pull request May 1, 2015

Closed

Improved shed_diff. #141

2 of 2 tasks complete
@erasche

This comment has been minimized.

Copy link
Member

erasche commented May 1, 2015

That all sounds fair. Definitely harkens back to your point about "reason for travis and a reason for buildbot".

If you'd like long running tests, the IUC jenkins bot is underutilised! :)

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented May 1, 2015

Thanks for the offer - it isn't so much about CI though. I definitely want that tool shed test running on Travis - another minute is not my concern. I think my other comment was about Galaxy and that minute would drawfs in comparision to the hours it would take to run Galaxy's functional tests. What I love is PLANEMO_SKIP_GALAXY_TESTS=1 nosetests is taking literally a couple seconds on my personal laptop. A hour matters in CI, a minute doesn't IMO - but a minute does matter during development. I love refactoring quickly and with impunity...

@erasche

This comment has been minimized.

Copy link
Member

erasche commented May 1, 2015

Gotcha, sounds good. Didn't realise that shed tests ran that quickly. Made a offhand assumption that it'd be 15-30m to really test all the shed related functionality and corner cases. (Especially if testing against shed@master and shed@dev)

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