Test & better document collect_cx_queues#13117
Conversation
ghost
added
| def test_cx_state(aggregator): | ||
| instance = copy.deepcopy(common.INSTANCE) | ||
| instance['collect_connection_state'] = True | ||
| check_instance = LinuxNetwork('network', {}, [instance]) |
There was a problem hiding this comment.
All the other tests now directly instantiate LinuxNetwork
| @mock.patch('datadog_checks.network.network.Platform.is_linux', return_value=True) | ||
| def test_returns_the_right_instance(is_linux, check): | ||
| check_instance = check({}) | ||
| assert isinstance(check_instance, LinuxNetwork) | ||
|
|
||
|
|
||
| @pytest.mark.skipif(not Platform.is_linux(), reason="Only works on Linux systems") | ||
| def test_collect_cx_queues(check, aggregator): | ||
| instance = copy.deepcopy(common.INSTANCE) | ||
| instance['collect_connection_queues'] = True | ||
| instance['collect_connection_state'] = True | ||
| check_instance = LinuxNetwork('network', {}, [instance]) | ||
|
|
||
| check_instance.check({}) | ||
|
|
||
| for metric in CONNECTION_QUEUES_METRICS + common.EXPECTED_METRICS: | ||
| aggregator.assert_metric(metric) | ||
|
|
||
|
|
||
| @pytest.mark.skipif(not Platform.is_linux(), reason="Only works on Linux systems") | ||
| def test_collect_cx_queues_when_ss_fails(check, aggregator): | ||
| instance = copy.deepcopy(common.INSTANCE) | ||
| instance['collect_connection_queues'] = True | ||
| instance['collect_connection_state'] = True | ||
| check_instance = LinuxNetwork('network', {}, [instance]) | ||
| with mock.patch('datadog_checks.network.check_linux.get_subprocess_output') as out: | ||
| out.side_effect = ss_subprocess_mock_fails | ||
| check_instance.check({}) | ||
|
|
||
| for metric in CONNECTION_QUEUES_METRICS + common.EXPECTED_METRICS: | ||
| aggregator.assert_metric(metric) |
There was a problem hiding this comment.
These are the new tests
|
|
||
| @pytest.mark.skipif(not Platform.is_linux(), reason="Only works on Linux systems") | ||
| def test_collect_cx_queues(check, aggregator): | ||
| instance = copy.deepcopy(common.INSTANCE) |
There was a problem hiding this comment.
I've been wondering what the difference is between this and providing INSTANCE as a per-test fixture. I'm guessing performance isn't an issue here, right?
There was a problem hiding this comment.
No difference, but since it's a constant I don't really see the need of using a fixture
There was a problem hiding this comment.
When we use an instance fixture it's typically the same thing, we simply abstract the copy.deepcopy out inside the fixture.
|
|
||
| @pytest.mark.skipif(Platform.is_windows(), reason="Only runs on Unix systems") | ||
| @mock.patch('os.listdir', side_effect=os_list_dir_mock) | ||
| @mock.patch('datadog_checks.network.check_linux.LinuxNetwork._read_int_file', side_effect=read_int_file_mock) |
There was a problem hiding this comment.
What is the team's general stance on using mock.patch? I personally try to only use it for legacy stuff, for new code I feel like I've designed something wrong if I have to mock deep inside a function instead of parametrizing (eg with dependency injection).
There was a problem hiding this comment.
We do not use dependency injection (except for test fixtures). In general I agree it is usually a smell if you need to use it although one might argue that some times extracting something to a separate module for the sake of correctness can be too much if that something is small and not shared with other things. I don't want to get too philosophical here but here all I'm doing is moving the tests from one file to another and change the SUT from a Network object to a LinuxNetwork object so I do not need to mock the Platform. This is very much legacy code still, I'm just giving it a little bit more structure.
There was a problem hiding this comment.
| for metric, value in iteritems(LINUX_SYS_NET_STATS): | ||
| aggregator.assert_metric(metric, value=value[0], tags=['iface:lo']) | ||
| aggregator.assert_metric(metric, value=value[1], tags=['iface:ens5']) | ||
| aggregator.reset() |
There was a problem hiding this comment.
Is this resetting necessary a the end of a test? (I know, this was there already)
There was a problem hiding this comment.
* move all linux tests to test_linux * Explicitly test NetworkLinux to avoid so much mocking * Add more unit tests da1f5e0
* move all linux tests to test_linux * Explicitly test NetworkLinux to avoid so much mocking * Add more unit tests
5 participants
Followup of #13109
collect_cx_queues option is only used in linux, moving it there and document that it is only used in linux.
Also it had no coverage so adding tests for it.
Moved all tests that are linux specific to test_linux.
Explicitly test the linux subclass to avoid so much mocking