Add functional tests for grub lib #104
Conversation
Switch to use update_config instead of save_config in Config.update function to ensure charm configs are updated and not overwritten. Fix wiring lines in _save_config function. Raise ApplyError if grub-mkconfig command failed, since it is failing also if there is wrong validation.
rgildein
force-pushed
the
grub/functional-tests
branch
from
c8ad9a8
to
d54a893
Compare
There was a problem hiding this comment.
Change itself looks reasonable. One small request (avoiding os.linesep) and one larger request (that we avoid mocking the filesystem in unit tests). However, it might be too much of a change in this PR to change how we're testing filesystem operations, so I'd be happy to keep that in mind for future work.
| ) | ||
| def test_update_validation_failure(self, _, mock_save, mock_apply): | ||
| @mock.patch.object(grub.Config, "_update") | ||
| @mock.patch.object(grub, "_update_config") |
There was a problem hiding this comment.
I still don't like all this mocking. It makes the tests more fragile as we're testing a lot more test internals. Ideally we could change these to use the real filesystem (in a temp directory) where possible, and only use mocking where necessary (for example, the subprocess update-grub calls). Local filesystem operations are fast and reliable, so don't need to be mocked.
There was a problem hiding this comment.
Hi @benhoyt, I agree, but at the same time the definition of unit tests is testing functionality of single function, because it's not good if I change one thing and it breaks half of unit tests. So for me it's really necessary to tests each function deeply with mocked everything else.
However at the beginning I mention that I agree with you and that's why I provide TestSmokeConfig where I mocked everything and TestFullConfig where I mocked only subprocess check_call. I think that it is good solution, because like this I test all scenarios for each function with mocking other functions and the general usage for .remove and .update function.
What do you think?
There was a problem hiding this comment.
I agree, but at the same time the definition of unit tests is testing functionality of single function, because it's not good if I change one thing and it breaks half of unit tests. So for me it's really necessary to tests each function deeply with mocked everything else.
"Unit testing" is a somewhat vague term, but just given the name, I don't think it necessarily means testing functions, but testing units (which could be modules, classes, methods). That said, I don't believe that debating the term is particularly helpful. I agree with you that we want to avoid "if I change one thing it breaks half of the tests".
But, as I see it, mocking internal helper functions is exactly how you'd get that problem. This test is mocking the internal _save_grub_configuration, _update, and _update_config functions. But those are implementation details, so if we change the name or implementation of any of those methods, this test will break. And that's fragile. That's why I advocate mocking as little as possible, and -- as much as possible -- only testing the external contract, not the implementation details. On the other hand, we probably do need to mock subprocess.check_call because it's an external service/program.
In any case, those are my thoughts. I'm not going to block this PR because the testing is not to my taste. We'll see if the tests end up being fragile and we can always change the testing approach later. And we can keep discussing what "unit testing" means over a beer in Riga. :-)
|
@benhoyt Can me moved CI to runner, which will be able to run VMs with lxd? It's mandatory to run integration tests for grub on non container machine, or potentially we can mocked that function. |
|
@rgildein Approved now. I'm not quite sure what you mean by "Can me moved CI to runner" -- can you clarify and/or propose that in the GitHub workflow YAML? |
If you check this line from logs form Integrations tests, it is saying that on that runner we could not create VM machine. I though that we are using self hosted runner on which is not possible to do this, but apparently this is an issue of GitHub runner where nested virtualization is disabled. I'll check how we can enable it. |
|
@rgildein Per your Mattermost message, you say:
I think we should do option 1 for now -- run Ubuntu integration tests directly on a VM runner. Fairly simple, and gets us at least some automated testing for this. |
rgildein
force-pushed
the
grub/functional-tests
branch
3 times, most recently
from
7a792eb
to
9f35388
Compare
Since the grub lib required to run integration tests on machine or VM, we are switch to running integration tests directly on GitHub runner. It was not possible to run it on VMs, since the GitHub runner do not supports nested virtualization.
rgildein
force-pushed
the
grub/functional-tests
branch
from
9f35388
to
d54f3e0
Compare
rgildein
force-pushed
the
grub/functional-tests
branch
from
d54f3e0
to
8e823c0
Compare
|
The linting error was fixed in #105. |
tests/integration/test_grub.py
Outdated
| with pytest.raises(grub.ValidationError): | ||
| grub_conf_2.update(config_2) | ||
|
|
||
| assert grub_conf_2.path.exists() is False |
There was a problem hiding this comment.
assert not grub_conf_2.path.exists() would be the Pythonic way to spell this. (Similar elsewhere.)
|
The code looks good. Looks like the Ubuntu integration tests are failing / timing out, though. |
The tests are not failing, but the "Setup operator environment" is failing. I think it's due some change in system config. We can check all tests and properly clean any configuration or simply run |
|
For the record, @NucciTheBoss said:
So we should probably get to the bottom of that at some point. In the meantime, I'm going to merge this. We can always improve / isolate CI later. |
_update_configfunction to update charm config file instead of overwriting it_save_configfunctiongrub-mkconfigfailed, since it is failing if wrong configuration was provided