fix #2902 #2994
fix #2902 #2994
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #2994 +/- ##
=======================================
Coverage 94.97% 94.98%
=======================================
Files 74 74
Lines 14124 14151 +27
=======================================
+ Hits 13414 13441 +27
Misses 710 710
Continue to review full report at Codecov.
|
| TTRN.add(sql, [artifact_info, job_id]) | ||
| else: | ||
| pending[artifact_info[0]][pname] = artifact_info[1] | ||
| elif pname == 'artifact': |
There was a problem hiding this comment.
Would you mind describing what's going on here?
There was a problem hiding this comment.
Sure, the create_command method in patch 58.py adds the delete_artifact and other Qiita internal commands; however, the artifact_id is in the parameter "artifact" vs. the parameter description (*); thus, if we want to add these commands to the database we need this special case.
(*) all regular jobs, except internal Qiita, have their parameters in this format: parameter_name: [parameter_type, value]; so normally, we just check that parameter_type is artifact but for internal jobs we need to check the actual parameter.
qiita_db/test/test_artifact.py
Outdated
| job.submit() | ||
| # let's wait for job | ||
| while job.status in ('queued', 'running'): | ||
| sleep(1) |
There was a problem hiding this comment.
I was looking around the repo and found qiita_core.testing.wait_for_processing_job. I think that would be able to do this.
| # n[1] is the object | ||
| for n in graph.nodes(): | ||
| if n[0] == 'job': | ||
| # ignoring internal Jobs |
There was a problem hiding this comment.
Does this need a new test case?
There was a problem hiding this comment.
This if is needed as a consequence of now adding the internal jobs to the database; my guess is that originally we didn't have it cause we didn't store them. In other words, the test for these method would need to change if we didn't have this if (as now the jobs will be returned here) AKA it's being tested here.
| TTRN.add(sql, [artifact_info, job_id]) | ||
| else: | ||
| pending[artifact_info[0]][pname] = artifact_info[1] | ||
| elif pname == 'artifact': |
There was a problem hiding this comment.
Sure, the create_command method in patch 58.py adds the delete_artifact and other Qiita internal commands; however, the artifact_id is in the parameter "artifact" vs. the parameter description (*); thus, if we want to add these commands to the database we need this special case.
(*) all regular jobs, except internal Qiita, have their parameters in this format: parameter_name: [parameter_type, value]; so normally, we just check that parameter_type is artifact but for internal jobs we need to check the actual parameter.
| # n[1] is the object | ||
| for n in graph.nodes(): | ||
| if n[0] == 'job': | ||
| # ignoring internal Jobs |
There was a problem hiding this comment.
This if is needed as a consequence of now adding the internal jobs to the database; my guess is that originally we didn't have it cause we didn't store them. In other words, the test for these method would need to change if we didn't have this if (as now the jobs will be returned here) AKA it's being tested here.
3 participants
Before this patch we didn't keep track of deleting jobs and this was the behavior:
The main issues were: