Add missing test for AckReportComputedTask #49
Conversation
kbeker
changed the title
| ) | ||
| message_timestamp = datetime.datetime.now(timezone.utc) | ||
| new_message = Message( | ||
| type = self.force_golem_data.__class__.__name__, |
There was a problem hiding this comment.
#20 has been merged. Please change it to numeric TYPE.
| @@ -355,6 +355,56 @@ def test_receive_return_http_204_if_no_messages_in_database(self): | |||
| self.assertEqual(response.content.decode(), '') | |||
| assert len(ReceiveStatus.objects.filter(delivered=False)) == 0 | |||
|
|
|||
| @freeze_time("2017-11-17 12:00:00") | |||
| def test_receive_should_get_ack_after_task_to_compute_is_not_after_deadline(self): | |||
There was a problem hiding this comment.
Please give it a more sensible name: test_receive_should_return_ack_if_the_receive_queue_contains_only_force_report_and_its_past_deadline.
The current name makes no sense because receive() does not get anything. It only returns a message.
| @@ -355,6 +355,56 @@ def test_receive_return_http_204_if_no_messages_in_database(self): | |||
| self.assertEqual(response.content.decode(), '') | |||
| assert len(ReceiveStatus.objects.filter(delivered=False)) == 0 | |||
|
|
|||
| @freeze_time("2017-11-17 12:00:00") | |||
There was a problem hiding this comment.
Please move this inside the function. You only need to freeze time around the client.post() call and I think this makes the test easier to undrstand because you can read the timestamps in order.
| self.assertEqual(response.status_code, 200) | ||
| self.assertIsInstance(decoded_ack_response, message.AckReportComputedTask) | ||
| self.assertEqual(decoded_ack_response.task_to_compute.compute_task_def['task_id'], self.task_to_compute.compute_task_def['task_id']) # pylint: disable=no-member | ||
| self.assertEqual(decoded_ack_response.task_to_compute.compute_task_def['deadline'], self.task_to_compute.compute_task_def['deadline']) # pylint: disable=no-member |
There was a problem hiding this comment.
- You can compare
computed_task_defas a whole. No need to compare the individual fields one by one. - You should actually check that the whole
TaskToComputemessage inside is unchanged and has the same signature. - You should compare
subtask_idtoo. - You should check that the message in queue has been marked as delivered.
- You should check that the timestamp of the ack message is within the allowed range. It should not be for example too far in the past because the client won't be able to decode it and will discard it instead.
kbeker
force-pushed
the
fix-add-missing-test-for-ack-response
branch
2 times, most recently
from
d9411b6
to
65eb1a0
Compare
|
Changes added. Ready for another review. |
kbeker
force-pushed
the
fix-add-missing-test-for-ack-response
branch
from
65eb1a0
to
6a69222
Compare
cameel
force-pushed
the
fix-add-missing-test-for-ack-response
branch
from
6a69222
to
099a123
Compare
kbeker
force-pushed
the
fix-add-missing-test-for-ack-response
branch
from
099a123
to
de3f980
Compare
cameel
force-pushed
the
fix-add-missing-test-for-ack-response
branch
from
de3f980
to
56f610d
Compare
|
Looks like the breakage reported by pylint was not from this branch but from #51 merged earlier. I have extracted the fix you added here into master. I did not notice this. If you see that it's master and not your branch that is broken, please tell me. master should always pass tests and have no warnings. |
Resolves #36 Added one missed test for AckReportComputedTask.