Add unit tests #14
Add unit tests #14
Conversation
| def test_list(self): | ||
| response = self.client.get(self.url) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertEqual(response.data['count'], 2) |
There was a problem hiding this comment.
In addition to checking the count, it could be nice to verify that it returns the expected virtual circuit objects
| self.assertEqual(response.data['count'], 2) | ||
|
|
||
| def test_get_200(self): | ||
| response = self.client.get(f'{self.url}{self.vc1.vcid}/') |
There was a problem hiding this comment.
is the trailing / required in the url? if not, it may be good to also test the case with no trailing slash
There was a problem hiding this comment.
Django's URL ends with a / by default, unless set otherwise. Here I am using the defaults so I will need to include the trailing /.
| self.assertEqual(len(response.data), 3) | ||
| self.assertIn('vcid', response.data) | ||
| self.assertIn('name', response.data) | ||
| self.assertIn('vlans', response.data) |
There was a problem hiding this comment.
what is the expected behavior here? it feels a little strange that a 400 error is returning virtual circuit data. maybe thats intentional/best practice for netbox plugins, but just wanted to verify..
There was a problem hiding this comment.
For POST request, these 3 parameters vcid, name, and vlans are required. If they're not provided, it will return a 400 error saying that the missing ones are required ("field_name": "field_value is required"). This check makes sure that the field names are returned in the response.
| self.assertEqual(response.data['name'], 'foo') | ||
| self.assertEqual(response.data['status'], VirtualCircuitStatusChoices.STATUS_PENDING_CONFIGURATION) | ||
| self.assertEqual(response.data['context'], 'bar') | ||
| self.assertEqual(len(response.data['vlans']), 0) |
There was a problem hiding this comment.
is it valid to be able to create a virtual circuit without any vlans? I thought a virtual circuit required vlans
There was a problem hiding this comment.
Yes it's possible to create a virtual circuit without any VLANs. This is actually the initial behavior of the plugin where one needs to create a virtual circuit first then assign VLANs later. Since that takes up 2 requests to do so, #11 allows to create a virtual circuit and assign VLANs at the same time, in a single request.
|
|
||
|
|
||
| class VirtualCircuitEndpointTestCase(TestCase): | ||
| def setUp(self): |
There was a problem hiding this comment.
A somewhat nit-picky/minimal suggestion is that in addition to (or possibly in place of) this setUp block, which runs before every test case, we can consider using setUpClass, which runs once before all tests run. I think that since we are creating the same user/token/client/url for each test, that may be a good place to put it so its only run once.
A second question I have here is whether or not there is a need to define a tearDown function to clean up any of the seed data, either between test cases to ensure each test case starts with the same data, or after the test class completes, to not pollute state for other test classes
There was a problem hiding this comment.
I've been trying to see if I can use setUpClass instead of setup but for some reason, most requests now return a 403 error. I haven't been able to figure out why that's the case so I am opening & tracking an issue and keeping this as it is for now.
On tearDown it seems like test data get cleaned up automatically after each test case. So it might not need to define one.
There was a problem hiding this comment.
oh, interesting. I just noticed that the base class is a django.test.TestCase not a unittest.TestCase, so its pretty likely there is that teardown functionality built in there. It might also be why setUpClass isn't working, if there are some other bits that are going on in the base implementation.
| def test_list(self): | ||
| response = self.client.get(self.url) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertEqual(response.data['count'], 2) |
There was a problem hiding this comment.
In addition to checking the count, would be good to verify we get the expected objects as well
| self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) | ||
|
|
||
| class VCVLANEndpointTestCase(TestCase): | ||
| def setUp(self): |
There was a problem hiding this comment.
same notes as for the previous test case
| def test_get_virtual_circuits(self): | ||
| self.assertEqual(self.vc1.vcid, 1) | ||
| self.assertEqual(self.vc1.name, 'VC 1') | ||
| self.assertEqual(self.vc1.status, VirtualCircuitStatusChoices.STATUS_PENDING_CONFIGURATION) | ||
| self.assertEqual(self.vc1.context, '') | ||
|
|
||
| self.assertEqual(self.vc2.vcid, 2) | ||
| self.assertEqual(self.vc2.name, 'VC 2') | ||
| self.assertEqual(self.vc2.status, VirtualCircuitStatusChoices.STATUS_CONFIGURED) | ||
| self.assertEqual(self.vc2.context, '') | ||
|
|
||
| self.assertEqual(self.vc3.vcid, 3) | ||
| self.assertEqual(self.vc3.name, 'VC 3') | ||
| self.assertEqual(self.vc3.status, VirtualCircuitStatusChoices.STATUS_PENDING_CONFIGURATION) | ||
| self.assertEqual(self.vc3.context, 'foo') | ||
|
|
||
| def test_get_vlans(self): | ||
| self.assertEqual(self.vlan1.vid, 1) | ||
| self.assertEqual(self.vlan1.name, 'VLAN 1') | ||
|
|
||
| self.assertEqual(self.vlan2.vid, 2) | ||
| self.assertEqual(self.vlan2.name, 'VLAN 2') | ||
|
|
||
| self.assertEqual(self.vlan3.vid, 3) | ||
| self.assertEqual(self.vlan3.name, 'VLAN 3') | ||
|
|
||
| def test_get_virtual_circuits_vlans(self): | ||
| self.assertEqual(self.vc1_vlan1.virtual_circuit, self.vc1) | ||
| self.assertEqual(self.vc1_vlan1.vlan, self.vlan1) | ||
|
|
||
| self.assertEqual(self.vc1_vlan2.virtual_circuit, self.vc1) | ||
| self.assertEqual(self.vc1_vlan2.vlan, self.vlan2) | ||
|
|
||
| self.assertEqual(self.vc2_vlan3.virtual_circuit, self.vc2) | ||
| self.assertEqual(self.vc2_vlan3.vlan, self.vlan3) |
There was a problem hiding this comment.
I'm not totally sure what this is testing, as its just verifying the state that was defined in the testcase setup. The name of the test implies that we are testing a "get" capability, but I don't see any getting happening.
If testing the side-effect of the call to create the object, then those calls to create shouldn't be done in the test case setup, but rather in the test case themselves, for the objects applicable to the test
|
|
||
|
|
||
| class VirtualCircuitTestCase(TestCase): | ||
| def setUp(self): |
There was a problem hiding this comment.
similar note as above for setUp vs setUpClass
This PR:
Makefilecommandtestto quickly setupdocker-composeand run tests.