Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unit tests for config.cc #3631

Merged
merged 4 commits into from May 28, 2020
Merged

Conversation

andir
Copy link
Member

@andir andir commented May 27, 2020

This adds unit tests for libutil/config.{hh,cc}, particularly the Config class and it's interaction with Setting. Since initially it had been a bit confusing how Setting and Config interact there are now a few additional comments in the header file.
In order to test the configuration file parsing (without solving the cross-platform temporary file problem) I had to do a minor refactoring of the Config::applyConfigFile method. The part that actually operates on the contents of the string has been factored out and is being tested with some basic examples. Actual testing with configuration files is still desirable but that is being done by the other (functional) tests.

While the tests cover most of the methods there is probably still room for improvement. I couldn't figure out how to test things like warnUnkownSettings (output capturing isn't supported by GTest and redirecting FDs didn't feel right) or reapplyUnknownSettings (see FIXME comments in the Diff).

andir added 4 commits May 27, 2020 17:47
This moves the actual parsing of configuration contents into applyConfig
which applyConfigFile is then going to call. By changing this we can now
test the configuration file parsing without actually create a file on
disk.
Provides some general overview on the mechanics of Config/Setting and
comments for the public methods of Config.
@gilligan gilligan mentioned this pull request May 28, 2020
16 tasks
@edolstra edolstra merged commit f60ce4f into NixOS:master May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants