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

Ready Integration Testing for Prime Time #3291

Merged
merged 11 commits into from Dec 15, 2016

Conversation

Projects
None yet
5 participants
@jmchilton
Copy link
Member

commented Dec 8, 2016

Integration tests were added in #2078 and a74cda6 - and expanded by @mvdbeek for Conda testing in b5fad4a.

From a74cda6, "IntegrationTestCases differ from functional tests in that they start up a Galaxy instance configured on a per test class basis - so this new class of "integration tests" can test configuration modalities of Galaxy that cannot be tested with traditional Galaxy functional tests."

This PR adds the ability to run integration tests via run_tests.sh and fixes up the Jenkins script I sketched out previously but didn't touch for executing them. It also refactors how integration tests can leverage utilities from the API test framework.

This PR adds a test case for various advanced job runner functionality - job resource parameters and re-submission of jobs based on hitting walltime and memory limits. These were previously not tested at all. In order to implement this testing I also enhanced the local job runner to leverage the job state handling framework implemented by @natefoo and previously only available to the asynchronous job runners.

xref #3245

@galaxybot galaxybot added this to the 17.01 milestone Dec 9, 2016

@jmchilton jmchilton force-pushed the jmchilton:integration_tests_0 branch 4 times, most recently from a1b7668 to 21294f9 Dec 12, 2016

@natefoo

This comment has been minimized.

Copy link
Member

commented Dec 12, 2016

Hooray for not having to duplicate the resource param parsing in every rule. Doing this probably should have been obvious but I'm never one for getting the obvious...

resource_params = {}
try:
resource_params_raw = param_values[ "__job_resource" ]
if resource_params_raw[ "__job_resource__select" ].lower() in [ "1", "yes", "true" ]:

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 14, 2016

Member

Maybe a modified one of this function?

This comment has been minimized.

Copy link
@jmchilton

jmchilton Dec 14, 2016

Author Member

I don't want a dependency between galaxy.web.framework.helpers and galaxy.jobs. I guess we don't want to expand the web stuff to allow 1 as well - we should be moving toward JSON booleans in the API - not strings for web stuff. So I would say we keep it like it is for now? If you really want me to modify it - I can switch this out for a generic tool handling boolean thing I guess - but I'd prefer not to.

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 14, 2016

Member

Not needed at all.

try:
resource_params_raw = param_values[ "__job_resource" ]
if resource_params_raw[ "__job_resource__select" ].lower() in [ "1", "yes", "true" ]:
for key, value in resource_params_raw.iteritems():

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 14, 2016

Member

This is not python3, but I guess it does not count here?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Dec 14, 2016

Member

We should use items() everywhere unless there is a performance concern, in which case there is six.iteritems() .

This comment has been minimized.

Copy link
@jmchilton

jmchilton Dec 14, 2016

Author Member

We should use items - I'll update this.

jmchilton added some commits Dec 3, 2016

Improved documentation and design of integration tests.
- Allow running through ``run_tests.sh``.
- Enable more verbose test errors for Conda tests.
- Refactor how integration tests depend on API testing (use a Mixin instead of subclassing conceptually conflicting TestCase classes).
Integration test for advanced job runner stuff.
Set of test cases that cover job resource handlers and resubmit job destinations.
Allow expressions in job_conf resubmission conditions.
Since we allow dynamic job runners clearly job_conf.xml must be considered a trusted document. Still I know that some are uneasy with eval generally so I only allow a very safe subset of Python expressions in this job_conf document.
Fix preservation of resubmits for resubmitted jobs.
Create an abstraction for the explicit handling of resubmits that is done during job recovery at startup.

xref 5539a08
Allow (and test) using expressions in job re-submission delays.
So you can backoff re-submission with ``delay="attempt * 30"`` for instance - which will cause a re-submission delay to increase by 30 seconds for each resubmission of a job.

@jmchilton jmchilton force-pushed the jmchilton:integration_tests_0 branch from 21294f9 to 334dfe1 Dec 14, 2016

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2016

Okay - so I have significantly expanded the scope of this PR. It now covers essentially all of #3293 as well.

  • There is a new job runner failure type (in addition to memory and walltime) called "unknown_error" that can be a re-submission condition.
  • The re-submit condition can now be a simple Python expression (a whitelisted expression despite the fact we absolutely allow arbitrary code execution in job_conf.xml in other ways).
  • The re-submit condition includes new variables for attempts, seconds_since_queued, and seconds_running to allow various re-submission combinations (re-submit long jobs, short jobs, X number of times, and logical combinations thereof).
  • The re-submit condition can now include a delay (in either absolute seconds or some expression - for instance using attempt or seconds_since_queued to implement back-off behaviors).
  • Integration tests for all of thee above.
@natefoo

This comment has been minimized.

Copy link
Member

commented Dec 15, 2016

💯

@natefoo natefoo merged commit 498dddb into galaxyproject:dev Dec 15, 2016

4 checks passed

api test Build finished. 243 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 131 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 22, 2017

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 22, 2017

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 22, 2017

@nsoranzo nsoranzo deleted the jmchilton:integration_tests_0 branch Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.