-
Notifications
You must be signed in to change notification settings - Fork 16
added new unit tests for the get method of handler "BarcodeUtilHandler" #159
Conversation
@@ -201,6 +201,9 @@ def get(self): | |||
project_names = db.getProjectNames() | |||
|
|||
# barcode exists get general info | |||
# TODO (Stefan Janssen): check spelling of "received", i.e. tests in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the DB, it is written "Received"
, but in the template it is used like
{% for bstatus in ['Recieved', ''] %} {% if bstatus == barcode_info['status'] %}
So I don't see how that condition can ever be satisfied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea... that's not good.
@@ -213,6 +216,8 @@ def get(self): | |||
if (barcode_details['obsolete'] == "Y"): | |||
# the barcode is obsolete | |||
div_id = "obsolete" | |||
# TODO: Stefan: why is that set here, as far as I see, this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur
response.body) | ||
|
||
# check if sequencing status is set to '' | ||
# (TODO: currently it is set to WAITING, which seems to be wrong!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is WAITING wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the following values in our DB for the column sequencing_status:
"''"
""
"FAILED_SEQUENCING"
"FAILED_SEQUENCING_1"
"FAILED_SEQUENCING_2"
"SUCCESS"
"WAITING"
Thus, I figure there should be a difference between a barcode marked as "waiting" and a barcode without any information, especially since there is the missing value option in the combo box <option value="" ></option>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, thanks
A flake8 error and it looks like a timeout again :/ |
Changes Unknown when pulling 85d53b3 on sjanssen2:unittest_barcode_util into * on biocore:master*. |
Changes Unknown when pulling d515e52 on sjanssen2:unittest_barcode_util into * on biocore:master*. |
self.assertEqual(response.code, 200) | ||
self.assertIn('Successfully updated barcodes in database', | ||
response.body) | ||
self.assertIn(response.code, [200, 599]) # either success, or time out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to have it retry on 599
instead of allowing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling that Travis imposes some limitations on the time-out length, because it always returns fine, i.e. with 200, when running this test on my local machine. I don't want to spend too much effort here to figure out how to make this happen within Travis.
But any recommendations are welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sounds good. On closer inspection, it could also be the case that EBI is throttling travis as well. I don't think there is an easy solution here.
data['postmark_date'] = '2015-06-25' | ||
data['scan_date'] = '2015-07-01' | ||
|
||
# TODO: Stefan Janssen: looks like we can successfully update a non |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
response.body) | ||
|
||
# test changing the barcode's project to a non existing one | ||
# TODO: Stefan Janssen: I think this should not result in a positive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
Just one question about the |
In order to address #158 I'd like to have unit tests in place. I think I found some odd things in the code and will correct them after the test PR has been accepted. Until now, I haven't touched the handler sources other than adding TODOs.