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

Scottx611x/td api fixes #1653

Merged
merged 13 commits into from Apr 3, 2017
Merged

Scottx611x/td api fixes #1653

merged 13 commits into from Apr 3, 2017

Conversation

scottx611x
Copy link
Member

Small fixes for ToolDefinition API code brought to light by writing documentation for it.

@codecov-io
Copy link

codecov-io commented Mar 28, 2017

Codecov Report

Merging #1653 into develop will decrease coverage by 0.65%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1653      +/-   ##
===========================================
- Coverage    37.64%   36.98%   -0.66%     
===========================================
  Files          369      263     -106     
  Lines        24317    20601    -3716     
  Branches      1260     1253       -7     
===========================================
- Hits          9153     7620    -1533     
+ Misses       15164    12980    -2184     
- Partials         0        1       +1
Impacted Files Coverage Δ
...r/management/commands/generate_tool_definitions.py 69.84% <91.66%> (-0.85%) ⬇️
refinery/annotation_server/utils.py 60.38% <0%> (-39.62%) ⬇️
...y/ui/source/js/dashboard/directives/width-fixer.js 64.28% <0%> (-35.72%) ⬇️
refinery/ui/source/js/chart/state.js 78.12% <0%> (-21.88%) ⬇️
...nery/ui/source/js/commons/list-graph/controller.js 16.31% <0%> (-15.05%) ⬇️
refinery/analysis_manager/admin.py 92.75% <0%> (-7.25%) ⬇️
refinery/core/search_indexes.py 41.6% <0%> (-1.67%) ⬇️
...inery/ui/source/js/commons/list-graph/directive.js 100% <0%> (ø) ⬆️
refinery/analysis_manager/urls.py 100% <0%> (ø) ⬆️
refinery/ui/source/js/commons/services/sharing.js 100% <0%> (ø) ⬆️
... and 123 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cf0725...d9bb73b. Read the comment docs.

"""
Mock of `get_workflow_list()`
Unserializes galaxy workflow response data that was serialized prior.
:filename: String containing filename of prior serialized Galaxy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't actually take a file name. I'd either update the docstring or (preferably) refactor the function to accept an actual file name (including the extension).

@@ -282,11 +282,13 @@ def get_workflow_list():
return workflow_list


def mock_get_workflow_list():
def mock_get_workflow_list(filename):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function simply reads a JSON file content and loads it into a Python object, so I'd name it accordingly and make it re-useable by allowing to specify any file path.

side_effect=mock_get_workflow_list):
with mock.patch(
'tool_manager.utils.get_workflow_list',
side_effect=[
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hackdna: I could also just nix the mock_get_workflow_list() and do the json.loads() in this side_effect array here? thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that's probably even better in this case.

@scottx611x scottx611x requested a review from hackdna March 31, 2017 13:02
with mock.patch('tool_manager.utils.get_workflow_list',
side_effect=mock_get_workflow_list):
invalid_workflows = open(
"tool_manager/test_data/invalid_galaxy_workflows.json"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRY: tool_manager/test_data/ should be stored in a variable.

@scottx611x scottx611x requested a review from hackdna April 1, 2017 01:01
@scottx611x scottx611x merged commit 24ec5d5 into develop Apr 3, 2017
@scottx611x scottx611x deleted the scottx611x/td_api_fixes branch April 3, 2017 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants