Yet another refactor

[chadell]

Proposing a new iteration to improve the usability of the library.

Proposed workflows, where the evaluate has an implicit validation of the reference_data per CheckType:

check = CheckType("exact_match")

data_to_evaluate = check.get_value(post_data, path, exclude)

reference_data = {
    "reference_data":  check.get_value(pre_data, path, exclude)
}

raw_result, bool_result = check.evaluate(data_to_evaluate, **reference_data)

check = CheckType("tolerance_match")

data_to_evaluate = check.get_value(post_data, path, exclude)

reference_data = {
    "reference_data":  check.get_value(pre_data, path, exclude),
    "tolerance": 10
}

raw_result, bool_result = check.evaluate(data_to_evaluate, **reference_data)

check = CheckType("parameter_match")

data_to_evaluate = check.get_value(post_data, path, exclude)

reference_data = {
    "mode":  "match",
    "params": {"1.1.1.1": "sdfsdf"}
}

raw_result, bool_result = check.evaluate(data_to_evaluate, **reference_data)

check = CheckType("parameter_match")

check.evaluate(my_var,

class ParameterMatch:
def evaluate(value, param1, param2, ):
self.validate(param1, param2)
... logic ...

[chadell]

Suggested change

	def evaluate(self, reference_value: dict, value_to_compare: Any) -> Tuple[Dict, bool]:
	def evaluate(self, value_to_compare: Any, **reference_value) -> Tuple[Dict, bool]:

[chadell]

Suggested change

	return self.hook_evaluate(self.args_class(reference_value), value_to_compare)
	return self.hook_evaluate(self.args_class(**reference_value), value_to_compare)

[pszulczewski]

Just remember about updating docstrings, or get someone to do that. I can volunteer.

[pszulczewski]

I would use validator_class for consistency.

[pszulczewski]

What do you think about adding:
self.validator_class.validate(**kwargs) in this abstract method? This would show the proper usage.

[pszulczewski]

It makes me think that using abstract class could simplify it and still enforce common interface.

from abc import ABC, abstractmethod


class CheckType(ABC):

    @abstractmethod
    def validate(self, **kwargs):
        # Validate logic
        pass

    @abstractmethod
    def evaluate(self, data, **kwargs):
        self.validate(**kwargs)
        # Evaluate logic
        pass


class MyCheck(CheckType):

    def validate(self, **kwargs):
        # check-type logic
        return

    def evaluate(self, data, **kwargs):
        self.validate(**kwargs)
        # check-type logic
        return

Definitely not something to do now, but just wanted to share this as an idea. IMO cleaner and less boilerplate code.

[lvrfrc87]

LGTM. Let's make this roll so we can move forward with the last check implementation :)

[chadell]

@pszulczewski @lvrfrc87 , the PR is almost ready, only 3 tests are failing, but I believe that maybe it's related to some change in the logic.

______________________________________________________________ test_checks[napalm_get_bgp_neighbors-tolerance-evaluate_args2-global.$peers$.*.*.ipv6.[accepted_prefixes,received_prefixes,sent_prefixes]-expected_result2] ______________________________________________________________

folder_name = 'napalm_get_bgp_neighbors', check_type_str = 'tolerance', evaluate_args = {'tolerance': 10}, path = 'global.$peers$.*.*.ipv6.[accepted_prefixes,received_prefixes,sent_prefixes]'
expected_result = ({'10.64.207.255': {'received_prefixes': {'new_value': 1100, 'old_value': 1000}}}, False)

    @pytest.mark.parametrize("folder_name, check_type_str, evaluate_args, path, expected_result", check_tests)
    def test_checks(folder_name, check_type_str, evaluate_args, path, expected_result):
        """Validate multiple checks on the same data to catch corner cases."""
        check = CheckType.init(check_type_str)
        pre_data, post_data = load_mocks(folder_name)
        pre_value = check.get_value(pre_data, path)
        post_value = check.get_value(post_data, path)
        actual_results = check.evaluate(post_value, pre_value, **evaluate_args)
    
>       assert actual_results == expected_result, ASSERT_FAIL_MESSAGE.format(
            output=actual_results, expected_output=expected_result
        )
E       AssertionError: Test output is different from expected output.
E         output: ({}, True)
E         expected output: ({'10.64.207.255': {'received_prefixes': {'new_value': 1100, 'old_value': 1000}}}, False)
E         
E       assert ({}, True) == ({'10.64.207.255': {'received_prefixes': {'new_value': 1100,\n                                          'old_value': 1000}}},\n False)
E         At index 0 diff: {} != {'10.64.207.255': {'received_prefixes': {'new_value': 1100, 'old_value': 1000}}}
E         Full diff:
E           (
E         +  {},
E         +  True,
E         -  {'10.64.207.255': {'received_prefixes': {'new_value': 1100,
E         -                                           'old_value': 1000}}},
E         -  False,
E           )

could you take it from here and merge it?

[lvrfrc87]

@chadell this line was missing.

[lvrfrc87]

@chadell build is failing but locally is ok. Is it something you can look at?