Closed mirceaulinic closed 6 years ago
I think this is a good idea.
Do we have any unit tests for these cases (validation from a file and then the new case validation from source directly provided to function)?
Right, we should maybe have a bot that pings me everytime I submit a PR to remind me about tests... I will write next week.
@mirceaulinic I can do the ping ;)
I like the PR. It would it make easier to integrate it in salt. Can I help something?
we just need tests to merge it. One should suffice.
I just had some time to look into.
I think we should also update the driver function itself. https://github.com/napalm-automation/napalm-base/blob/enh-validate/napalm_base/base.py#L1541
def compliance_report(self, validation_file='validate.yml'):
"""
Return a compliance report.
Verify that the device complies with the given validation file and writes a compliance
report file. See https://napalm.readthedocs.io/en/latest/validate.html.
"""
return validate.compliance_report(self, validation_file=validation_file)
Questen 1: Do we need the default value validate.yml
? If yes, the validation_file
will always be set and we should invert the logic of testing if validation_file
or validation_source
is set. https://github.com/napalm-automation/napalm-base/blob/enh-validate/napalm_base/validate.py#L158
if validation_file:
validation_source = _get_validation_file(validation_file)
@dbarrosop mentioned one test should suffice. An other option would be to extend each validation test with a second assert. https://github.com/napalm-automation/napalm-base/blob/enh-validate/test/unit/validate/test_validate.py#L31
assert expected_report == actual_report, yaml.safe_dump(actual_report)
validate_source = _read_yaml(os.path.join(mocked_data, "validate.yml"))
actual_report_from_source = device.compliance_report(validation_source=validate_source)
assert expected_report == actual_report, yaml.safe_dump(actual_report_from_source)
Questen 2: Do you like more then one assert
per test function or not?
I'm sorry for the mess. I'm trying to get the feeling of the community and I will contribute more and more in the future. You all do a great job!
I think we should also update the driver function itself. https://github.com/napalm-automation/napalm-base/blob/enh-validate/napalm_base/base.py#L1541
You are totally right. We should do some change there.
Do we need the default value validate.yml? If yes, the validation_file will always be set and we should invert the logic of testing if validation_file or validation_source is set
I think we should make it consistent with other methods like this one:
def load_merge_candidate(self, filename=None, config=None):
So we should probably do in the class:
def compliance_report(self, filename=None, compliance_rules=None):
It's going to be a backwards incompatible change so we should mark it as such.
Questen 2: Do you like more then one assert per test function or not?
I don't mind multiple asserts if the asserts are related to what you are testing. We have tests with many asserts already. The problem with your solution is that a test testing X might break by something unrelated which is confusing and misleading. For example, if I break loading the rules from a string somehow I will break the tests that verify that the strict
mode works fine as well.
Having the validation format in a single place makes the validation constrained by placing the content only on a single file, on the local disk. There are many applicabilities where we need to test with remote content, or any other source. This is making the validator more flexible