test: increase coverage of unit tests generator classes #1659
Conversation
hetangmodi-crest
changed the title
| @@ -58,6 +82,32 @@ def test_set_attribute_with_no_pages( | |||
| assert not hasattr(default_xml, "default_xml_content") | |||
|
|
|||
|
|
|||
| @patch( | |||
| "splunk_add_on_ucc_framework.generators.xml_files.DefaultXml._set_attributes", | |||
There was a problem hiding this comment.
I commented out this patch and the test passes. Is it needed and expected?
There was a problem hiding this comment.
Yes, it would work, but I am using the Mockist style unit testing - that the function calls do not go outside that function and I have the complete control of all returned objects from function calls made.
If we comment it, the parent's methods would be called, which is the way of Classical style unit testing, both would work.
| new_callable=list, | ||
| ) | ||
| @patch("splunk_add_on_ucc_framework.generators.file_generator.logger") | ||
| def test_begin_if_empty_dict( |
There was a problem hiding this comment.
What should this test check? Which dict is empty?
There was a problem hiding this comment.
This test case checks the mentioned condition. It increases the coverage to 100%.
| ucc_dir=ucc_dir, | ||
| addon_name=ta_name, | ||
| ) | ||
| alert_html._set_attributes() |
There was a problem hiding this comment.
This call is redundant as it is already called in the init
There was a problem hiding this comment.
Updated the code.
Removed explicit calls to _set_attributes() wherever not required.
|
|
||
| server_conf._gc_schema = None | ||
|
|
||
| server_conf._set_attributes() |
There was a problem hiding this comment.
Here we will be requiring to explicitly call _set_attributes() as after creating an instance of ServerConf we are setting _gc_schema to None.
| ): | ||
| global_config_for_alerts = MagicMock() | ||
| global_config_for_alerts.alerts = [] | ||
| mock_normalize.normalize.return_value = {"schema.content": {"modular_alerts": []}} |
There was a problem hiding this comment.
The test passes without this mock. Is it needed?
There was a problem hiding this comment.
I added it as I was following Mockist style unit test, however the test works without it too. Shall I remove it if we are following Classical style unit test?
There was a problem hiding this comment.
I think the logic here can be simplified a lot - FileGenerator, XMLGenerator and HTMLGenerator can be deleted and the files generation logic can be delegated to the implementation classes themselves.
XMLGenerator and HTMLGenerator are converting None to {"": ""} or taking what's there if not None. DefaultXml should be able to do it on it's own.
FileGenerator is a bit different story but yet similar. Implementation classes generate 2 files - .conf and .conf.spec. But actually maybe only half of the classes generate both those files. Can we make those implementation classes implement generate method themselves?
Something like (I did not test if that works but the idea should be clear)
def generate(self) -> Dict[str, str]:
conf_file = self.generate_conf()
conf_spec_file = self.generate_conf_spec()
return {
**conf_file,
**conf_spec_file,
}
This can be used in AlertActionsConf for example.
@kkedziak-splunk I would appreciate your opinion on this matter.
| @patch( | ||
| "splunk_add_on_ucc_framework.generators.conf_files.ConfGenerator.generate_conf_spec" | ||
| ) | ||
| def test_generate_conf_and_conf_spec_is_absent( |
There was a problem hiding this comment.
This test case is essentially the same as the one below (test_generate_conf).
There was a problem hiding this comment.
Yes, I have removed the test case.
Code Coverage 🎉
|
Issue number:
ADDON-79015
PR Type
What kind of change does this PR introduce?
Summary
Changes
Added unit test cases for the
generatorsmodule to achieve 100% test coverage.User experience
No change in user experience.
Checklist
If an item doesn't apply to your changes, leave it unchecked.
Review
Tests
See the testing doc.
Demo/meeting:
Reviewers are encouraged to request meetings or demos if any part of the change is unclear