[REF] tests : Refactoring of the tests #296
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
jaeschwa
force-pushed
the
master-improve-testing-scju
branch
from
714bd14
to
3cd2b53
Compare
Enhance Testing Framework, Refactor XML Parsing with etree,
and Enforce Field Constraints. To streamline the testing process
and reduce the burden of code reviews, this commit introduces a
multitude of enhancements and refactors within the codebase.
Key improvements include:
- New tests for manifests and xml
- A shift from regex-based to etree-based XML parsing
- Implementation of strict field constraints
- Added Extensive Tests:
- Implemented numerous new tests to simplify testing and reduce review complexity.
- Expanded coverage across multiple modules to ensure robustness.
- Refactored XML Parsing:
- Replaced regex-based parsing with `etree` from the `lxml`
library for improved accuracy and maintainability.
- Enhanced error handling and logging for XML processing.
- Implemented `check_fields` and `FIELDS_NOT_TO_SET_DIRECTLY`:
- Introduced the `check_fields` function to validate that certain
fields are not set directly in XML data because those are readonly
fields computed fields.
- Defined `FIELDS_NOT_TO_SET_DIRECTLY` to specify fields across various
model fields that should be defined in their related model.
- Updated Manifest Validation:
- Strengthened manifest checks to include mandatory keys and correct
dependency ordering.
- Ensured theme dependencies are listed last and dependencies are
alphabetically sorted.
These changes significantly improve the maintainability and reliability
of the codebase by enhancing test coverage, ensuring proper XML handling,
and enforcing strict field constraints. The refactored XML parsing with
`etree` provides a more robust solution, while the `check_fields` mechanism
safeguards against improper data manipulation. Enhanced manifest validations
and logging facilitate smoother development and deployment processes.
jaeschwa
force-pushed
the
master-improve-testing-scju
branch
from
3cd2b53
to
97142ec
Compare
vava-odoo
reviewed
Nov 21, 2024
| @@ -78,7 +78,7 @@ def test_cloc_exclude_view(self): | |||
| for module in self.installed_modules: | |||
| for cloc_entry in c.modules.get(module, {}): | |||
| message = "The view '%s' is counted in the maintenance lines. " % cloc_entry | |||
| message += "Please add a '__cloc_exclude__' entry in 'ir_model_data'." | |||
| message += "Please add a '__cloc_exclude__' entry in 'manifest'." | |||
There was a problem hiding this comment.
useless diff (still wrong and easier for possible conflicts aftewards)
| "Wrong %s '%r' in manifest, it should be %s" % (key, value, expected_value), | ||
| ) | ||
| self.assertIsInstance(value, expected_type, f"Wrong type for '{key}' in {module}, expected {expected_type}") | ||
| if expected_value and key not in ['description']: |
There was a problem hiding this comment.
expected_value of description falsy (empty string). Why do you need it?
Suggested change
| if expected_value and key not in ['description']: | |
| if expected_value: |
| ) | ||
| self.assertIsInstance(value, expected_type, f"Wrong type for '{key}' in {module}, expected {expected_type}") | ||
| if expected_value and key not in ['description']: | ||
| self.assertEqual(value, expected_value, f"Incorrect value for '{key}' in {module}") |
There was a problem hiding this comment.
Suggested change
| self.assertEqual(value, expected_value, f"Incorrect value for '{key}' in {module}") | |
| self.assertEqual(value, expected_value, f"Incorrect value for '{key}' in manifest of {module}") |
Comment on lines
-48
to
-51
| def _test_manifest_keys(self, module, manifest_data): | ||
| super()._test_manifest_keys(module, manifest_data) | ||
| for key in MANDATORY_KEYS: | ||
| self.assertIn(key, manifest_data, "Missing key %s in manifest" % key) |
There was a problem hiding this comment.
This test was checking that the key was defined + the super was checking that no other unknown key is defined
| def _validate_manifest(self, module, manifest_data): | ||
| # Check manifest keys and values | ||
| for key, expected_value in MANDATORY_KEYS.items(): | ||
| value = manifest_data.get(key) |
There was a problem hiding this comment.
if not value, missing key
Comment on lines
-194
to
-195
| # if s.count("x_studio"): | ||
| # _logger.warning("Please remove 'studio' from 'x_studio' in %s.", file_name) |
There was a problem hiding this comment.
This would be nice to restore, now that the fix of company_id is merged
Comment on lines
+567
to
+569
| field_models = root.xpath(f"//field[@model='{model}']") | ||
| function_models = root.xpath(f"//function[@model='{model}']") | ||
| if records and not field_models and not function_models and noupdate: |
There was a problem hiding this comment.
Suggested change
| field_models = root.xpath(f"//field[@model='{model}']") | |
| function_models = root.xpath(f"//function[@model='{model}']") | |
| if records and not field_models and not function_models and noupdate: | |
| if records and noupdate: |
Comment on lines
+575
to
+579
| for model in MODELS_NOT_TO_UPDATE: | ||
| records = root.xpath(f"//record[@model='{model}']") | ||
| field_models = root.xpath(f"//field[@model='{model}']") | ||
| function_models = root.xpath(f"//function[@model='{model}']") | ||
| if records and not field_models and not function_models and not noupdate: |
There was a problem hiding this comment.
I wonder if we shouldn't remove the MODELS_NOT_TO_UPDATE list and assume everything else should be noupdate (no huge list to maintain).
We just need to find records that are not from a model in the other list and not noupdate.
Comment on lines
+587
to
+591
| try: | ||
| root = etree.fromstring(xml_content.encode('utf-8')) | ||
| except etree.XMLSyntaxError as e: | ||
| _logger.error("XML syntax error in file %s: %s", filename, e) | ||
| return |
There was a problem hiding this comment.
Since we're using etree in almost all methods, this could be done once in the main one.
| _logger.warning( | ||
| "Model %s should not be updated, please add 'noupdate=\"1\"' in the header of %s.", | ||
| model, | ||
| filename, | ||
| ) | ||
|
|
||
| def _check_useless_models(self, s, filename): | ||
| def _check_useless_models(self, xml_content, filename): |
There was a problem hiding this comment.
This method can be removed in my opinion
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enhance Testing Framework, Refactor XML Parsing with etree, and Enforce Field Constraints. To streamline the testing process and reduce the burden of code reviews, this commit introduces a multitude of enhancements and refactors within the codebase. Key improvements include:
New tests for manifests and xml
A shift from regex-based to etree-based XML parsing
Implementation of strict field constraints
Added Extensive Tests:
Refactored XML Parsing:
etreefrom thelxmllibrary for improved accuracy and maintainability.Implemented
check_fieldsandFIELDS_NOT_TO_SET_DIRECTLY:check_fieldsfunction to validate that certain fields are not set directly in XML data because those are readonly fields computed fields.FIELDS_NOT_TO_SET_DIRECTLYto specify fields across various model fields that should be defined in their related model.Updated Manifest Validation:
These changes significantly improve the maintainability and reliability of the codebase by enhancing test coverage, ensuring proper XML handling, and enforcing strict field constraints. The refactored XML parsing with
etreeprovides a more robust solution, while thecheck_fieldsmechanism safeguards against improper data manipulation. Enhanced manifest validations and logging facilitate smoother development and deployment processes.