Port SSH tests to new framework #3265
Conversation
tests-ng/plugins/sshd.py
Outdated
| result.returncode == 0 | ||
| ), f"Expected return code 0, got {result.returncode}" | ||
| self._sshd_config_text = result.stdout | ||
| self._sshd_config = None |
There was a problem hiding this comment.
What's the point of splitting the exec and parse stage? Like why already run the command here, but then only parse it into a dict on demand? I'd say either always read and parse the config at __init__ time and make read_config a simple getter method, or move loading and parsing fully into the read_config function without any caching
There was a problem hiding this comment.
moved the logic to init
not sure if this is 'pythonic', at least in java it is typically not appreciated to do 'real work' in the constructor
I don't have a strong opinion on this
There was a problem hiding this comment.
I don't have much opinion on in constructor or in getter either, just having half the work in one place and half in the other isn't great 😅
tests-ng/plugins/utils.py
Outdated
| return a.casefold() == b.casefold() | ||
|
|
||
|
|
||
| def get_normalized_sets( |
There was a problem hiding this comment.
not sure a function that always handles 2 sets is the best abstraction for a utility function, since that is pretty specific to this usecas.
There was a problem hiding this comment.
agree, did not think of it in this generic way, just wanted to get rid of the details in the test. should be possible to do it with an arbitrary number of sets I think
| @pytest.mark.booted | ||
| @pytest.mark.root | ||
| @pytest.mark.feature("ssh") | ||
| @pytest.mark.parametrize("sshd_config_item", required_sshd_config) |
There was a problem hiding this comment.
Why do we need parameterize here? Wouldn't it be easier to read to just have a simple test case that compares the dicts to each other? that would probably be more readable then having the parameterize do this per element.
There was a problem hiding this comment.
The advantage I see with parametrize is that it is easier to see why a test failed
Example with all tests working (nice to see which properties were tested)
Example with failing test
I am open to changing this, I do see the upside of comparing the whole dict in one go.
There was a problem hiding this comment.
ok, that's a good point and the nicer test output probably outweighs the slightly more complex syntax 👍
| assert is_set(actual_value), f"{actual_value} should be a set" | ||
| actual_set, expected_set = get_normalized_sets(actual_value, expected_value) | ||
| missing_sshd_configuration = expected_set - actual_set | ||
| assert not missing_sshd_configuration, f"{sshd_config_item}: missing values {missing_sshd_configuration}" |
There was a problem hiding this comment.
Is it intended, that we only fail on entries missing in the set, and ignore additional set entries in the actual_set?
There was a problem hiding this comment.
If not, then we could just move this into the equals_ignore_case making this function able to compare both strings and sets, then we don't even need a get_normalized_sets function at all. That would obviously require to also consider additional entries in the actual_set as errors.
There was a problem hiding this comment.
yes
I think this is what the current tests do (unless I am misremembering badly), and also I think it makes sense to allow a diff that way. I view the expected as 'this has to be in, but it is okay if there is more'.
Convert test to assert desired properties are part of the sshd config.
Part of #3160