search

genome / ptero-workflow

Client-facing API for the PTero system
1 stars 6 forks source link

Url parse #249

Closed mkiwala closed 8 years ago

mkiwala commented 8 years ago

Fixes #247 and adds logging to help understand failing model tests with spawned workflows.

This PR is missing a negative test to confirm that InvalidExecutionUrlError is raised when we expect it to be raised.

Also, the urls.py could be improved by having the 'format' and 'parser' strings auto-generated from the 'url' string (eg, '/executions/int:execution_id').

davidlmorton commented 8 years ago

It should be easy to add tests for url_parse directly, I think we should add those before we merge this PR.

mkiwala commented 8 years ago

I added a test for url_parse. Maybe we can get this into staging this weekend and run the model tests.

davidlmorton commented 8 years ago

I leave that up to you. I ran the updated (more strict) tests against production and confirmed that the bug that is preventing spawned workflows isn't in production. I'm not sure it is important enough that it cannot wait until Monday.

mkiwala commented 8 years ago

Tests are currently failing on this PR. I suspected whatever the cause of these failures is, it is not the code introduced in this PR. To demonstrate that the failures are not due to code on this PR, I had travis-ci re-run a previously successful test on this PR (the test run associated with cd0fb89).

davidlmorton commented 8 years ago

It is interesting that they seem to be failing on anything needing the shell-command service, and that they were both working on the same test 'parallel_by_dag' when they timed out. I'm not sure what's going on, but I was able pass tests after restarting the travis CI build on another open PR on this repo (PR #237 Travis CI build https://travis-ci.org/genome/ptero-workflow/builds/103694812).

mkiwala commented 8 years ago

Ok, it's better now. I re-ran the latest test on the PR, and the problem went away.

Also, I had first noticed this problem in the development environment. The problem is no longer occurring there either.

I think all of the comments on this PR have been addressed.