test: cover conf module #27

Merged

zomop merged 1 commit from test/25-cover-conf-module into master

2026-10-03 23:46:16 -03:00

zomop commented

2026-10-03 13:26:26 -03:00

(Migrated from github.com)

No description provided.

marcos-mendez (Migrated from github.com) approved these changes

2026-10-03 23:45:43 -03:00

marcos-mendez (Migrated from github.com) left a comment

Hi @zomop, thanks a lot for this, and welcome to Keel Linux!

I ran it locally: all 13 new tests pass, the full confconsole suite passes, and conf.py goes from 0% to 100% line and branch coverage, so adding conf to the 100% gate is right. The tests are clear, well named, and check real behaviour (parsing, rejecting bad lines, keeping comments when writing). That is exactly what we want.

Two small things, not blockers, but worth knowing for next time:

Testing through the public interface. test_load_conf_ignores_a_missing_explicit_file and test_set_default_nic_creates_a_missing_configuration_file build the object with object.__new__ and call _load_conf() directly. That ties the test to the class's internals: if someone refactors Conf.__init__, these tests break even though the behaviour is the same. A sturdier way is to reach the same case through the public path. For example, point conf.path at a temporary directory with monkeypatch, then call conf.Conf(), so the test only knows what a user of the module knows.
Don't freeze surprising behaviour; flag it. test_set_default_nic_replaces_existing_setting_and_preserves_other_lines asserts that when default_nic appears twice, both lines get rewritten. That is what the code does today, but it is probably not what anyone intended: the duplicate should likely collapse to one line. When a test uncovers behaviour like this, it is better to keep the test to the clear case and open an issue describing the oddity, so the test doesn't lock a bug in place. Would you like to open that issue?

Once CI is green I'll merge. Thanks again, and feel free to pick up another good first issue, or ask anything in Discussions.

Hi @zomop, thanks a lot for this, and welcome to Keel Linux! I ran it locally: all 13 new tests pass, the full confconsole suite passes, and conf.py goes from 0% to 100% line and branch coverage, so adding conf to the 100% gate is right. The tests are clear, well named, and check real behaviour (parsing, rejecting bad lines, keeping comments when writing). That is exactly what we want. Two small things, not blockers, but worth knowing for next time: Testing through the public interface. test_load_conf_ignores_a_missing_explicit_file and test_set_default_nic_creates_a_missing_configuration_file build the object with object.__new__ and call _load_conf() directly. That ties the test to the class's internals: if someone refactors Conf.__init__, these tests break even though the behaviour is the same. A sturdier way is to reach the same case through the public path. For example, point conf.path at a temporary directory with monkeypatch, then call conf.Conf(), so the test only knows what a user of the module knows. Don't freeze surprising behaviour; flag it. test_set_default_nic_replaces_existing_setting_and_preserves_other_lines asserts that when default_nic appears twice, both lines get rewritten. That is what the code does today, but it is probably not what anyone intended: the duplicate should likely collapse to one line. When a test uncovers behaviour like this, it is better to keep the test to the clear case and open an issue describing the oddity, so the test doesn't lock a bug in place. Would you like to open that issue? Once CI is green I'll merge. Thanks again, and feel free to pick up another good first issue, or ask anything in Discussions.

zomop commented

2026-10-04 01:09:50 -03:00

(Migrated from github.com)

Thanks so much for the detailed review and feedback! Really appreciate it, and I’m glad to have contributed to Keel Linux!