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 #3571

Merged
merged 12 commits into from May 8, 2020
Merged

Add unit tests #3571

merged 12 commits into from May 8, 2020

Conversation

gilligan
Copy link
Contributor

@gilligan gilligan commented May 6, 2020

Overview
This PR adds unit tests for libutil using https://github.com/google/googletest - The idea was to start somewhere and see how well writing tests works out and then open a PR to get some feedback and also some help.

Motivation
For now Nix has only ever had functional tests. They are super useful and can catch a lot of problems but unit tests can provide added benefits like documentation and making it easier to refactor or port code.

Notes
I picked some simple module (libutil) for testing first. There are going to be other parts of the code base which are more tricky to test and in various cases it just won't make any sense to try to do so. That being said it would be nice to gradually improve the amount of tests and to encourage contributors to provide tests when they change code/add features.

TODOs

  • Properly embed this into the build rules (tests/unit-tests/local.mk is a total hack)
  • Execute during nix build and/or during CI
  • Address the "XXX" comments in the tests

This is a proof on concept to evaluate writing unit tests for Nix using
google test (https://github.com/google/googletest).

In order to execute tests:

$ make unit-tests
$ ./unit-tests

The Makefile rules for `unit-tests` is a complete hack.
No need to use `c_str()` in combination with `ASSERT_STREQ`.
It's possible to just use ASSERT_EQ on std::string
The function isn't being used anywhere so it seems safe to remove
@gilligan
Copy link
Contributor Author

gilligan commented May 7, 2020

@Kha thanks, fixed!

@edolstra
Copy link
Member

edolstra commented May 8, 2020

@gilligan I've integrated the tests into make check. I've also moved them to src/libutil/tests.

Adjusted the doc comment for `dirOf` to reflect the implementation
behavior.
Add note about removal of trailing slashes in the doc comment of
baseNameOf and enabled the test.
Update comment and enable the test
@gilligan gilligan changed the title WIP: Add unit tests Add unit tests May 8, 2020
@gilligan
Copy link
Contributor Author

gilligan commented May 8, 2020

@edolstra From my point of view this would now be ready to be merged.

There are 2 remaining XXX in the test and i'm happy to address them in whatever way you prefer. I would however also be Okay with merging this first and addressing those separately.

Anything else you would like to see done to get this over the finishing line?

@edolstra edolstra merged commit d3d8186 into NixOS:master May 8, 2020
@bhipple
Copy link
Contributor

bhipple commented May 8, 2020

This is phenomenal! Thanks guys!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-3/7154/1

@gilligan gilligan mentioned this pull request May 25, 2020
16 tasks
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

5 participants