API Changes and CO Timestamp support #4
Conversation
pradeepsixer
commented
Dec 26, 2017
pradeepsixer
commented
- Updated the API to support customizations by the API consumer
- Added support for Created and Closed Timestamps for Change Requests.
1. Updated the API to support customizations by the API consumer 2. Added support for Created and Closed Timestamps for Change Requests.
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
==========================================
+ Coverage 92.04% 94.26% +2.21%
==========================================
Files 6 7 +1
Lines 88 122 +34
Branches 5 6 +1
==========================================
+ Hits 81 115 +34
Misses 7 7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I'm only reviewing the tests for a first pass, so that I can be sure they all make sense. To that end, I've made several comments about potential improvements.
One other thought I had is, does it perhaps make sense to split these tests out into multiple TestCase classes? That way there could be slightly better logical grouping of them, and more focused setUp() methods for each. Just a thought.
| fake_resource = mock.MagicMock() | ||
| fake_resource.create.return_value = fake_insert_retval | ||
|
|
||
| self.mock_pysnow_client.resource.return_value = fake_resource | ||
| mock_pysnow.Client.return_value = self.mock_pysnow_client |
There was a problem hiding this comment.
Can you get rid of the self.mock_pysnow_client? Since it's just an empty mock, it's not extremely useful, and it's making this logic a bit hard to follow.
There was a problem hiding this comment.
The self.mock_pysnow_client is being used in almost all the test cases. So instead of instantiating it every time within the test cases themselves, I think its better to have it at the setUp() method.
| last_co = ChangeRequest.objects.last() | ||
|
|
||
| mock_get_snow_group_guid.assert_called_with('assignment_group') | ||
| self.assertEqual(co.pk, last_co.pk) |
There was a problem hiding this comment.
It appears that two separate behaviors are being tested here; one to make sure that the return value of the create is as expected, and one to ensure that the object is added to the db. If that is the case, these should probably be split into separate tests.
There was a problem hiding this comment.
With the line 71 gone, it focuses only on checking the values of the persistence object created.
testapp/tests.py
Outdated
| expected_payload['type'] = 'normal' | ||
| expected_payload['assignment_group'] = 'bar' | ||
| expected_payload['short_description'] = 'Title' | ||
| expected_payload['description'] = 'Description' |
There was a problem hiding this comment.
Is an OrderedDict actually necessary here? I'm guessing that assert_called_with cares about the order?
If so, this could be cleaned up a bit:
expected_payload = OrderedDict({
'type': 'normal',
'assignment_group': 'bar',
'short_description': 'Title',
'description': 'Description'
})The same applies below for payload.
testapp/tests.py
Outdated
| expected_payload['short_description'] = 'Title' | ||
| expected_payload['description'] = 'Description' | ||
| expected_payload['type'] = 'standard' | ||
| expected_payload['assignment_group'] = 'bar' |
There was a problem hiding this comment.
Same notes apply here about the OrderedDict.
testapp/tests.py
Outdated
| expected_payload['type'] = 'standard' | ||
| expected_payload['assignment_group'] = 'bar' | ||
|
|
||
| self.change_request_handler.group_guid_dict['assignment_group'] = 'bar' |
There was a problem hiding this comment.
I'm not particularly a fan of this. This seems like a bit of a TDD anti-pattern. With TDD, tests should be written before code, but this line is quite obviously written after the code, because it implies, and requires, an intimate knowledge of the code internals. Is there a different way this can be controlled? Perhaps using django.test.override_settings?
testapp/tests.py
Outdated
| fake_resource = mock.MagicMock() | ||
| fake_resource.create.return_value = fake_insert_retval | ||
|
|
||
| self.mock_pysnow_client.resource.return_value = fake_resource | ||
| mock_pysnow.Client.return_value = self.mock_pysnow_client | ||
|
|
||
| with self.assertRaises(ChangeRequestException): |
There was a problem hiding this comment.
Since this is checking for the exact same exception as above, generated by the same method call, I think this should also verify that the proper error message is being raised, with six.assertRaisesRegex().
testapp/tests.py
Outdated
| self.mock_pysnow_client.resource.return_value = fake_resource | ||
| mock_pysnow.Client.return_value = self.mock_pysnow_client | ||
|
|
||
| with self.assertRaises(ChangeRequestException): |
There was a problem hiding this comment.
The same notes about the assert apply here.
testapp/tests.py
Outdated
|
|
||
| def test_get_snow_group_guid_cached_result(self, mock_pysnow): | ||
| self.change_request_handler.group_guid_dict['foo'] = 'bar' | ||
| self.assertEqual(self.change_request_handler.get_snow_group_guid('foo'), 'bar') |
There was a problem hiding this comment.
This test has the same code smell as I mentioned above with needing to have intimate knowledge of the inner workings of the code. I think it would be better to mock the pysnow result, and ensure that pysnow is only called on the first attempt to retrieve the guid. This same technique should probably be used in other tests. In fact, this could even justify the continued use of self.mock_pysnow_client, since that would be functionality which would be necessary for many of the tests.
|
This PR fixes #2 |
1. Replaced usage of OrderedDict usage with dictionary. 2. Rewritten test cases that required internal working knowledge. 3. assertRaises replaced with assertRaisesRegex
|
The updates look great! Aside from one issue: |
Added 'six' package for assertRaisesRegex shim for Python 2.7
