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 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.
tests/unit-tests/test-util.cc Outdated Show resolved Hide resolved
tests/unit-tests/test-util.cc Outdated Show resolved Hide resolved
tests/unit-tests/test-util.cc Outdated Show resolved Hide resolved
tests/unit-tests/test-util.cc Outdated Show resolved Hide resolved
tests/unit-tests/test-util.cc Outdated Show resolved Hide resolved
tests/unit-tests/test-util.cc Outdated Show resolved Hide resolved
tests/unit-tests/test-util.cc Outdated Show resolved Hide resolved
gilligan added 2 commits May 7, 2020
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
@Kha

This comment has been minimized.

Copy link

Kha commented on tests/unit-tests/test-util.cc in 987b3d6 May 7, 2020

I believe you can even drop the std::string( here since string_views also implement == and <<, where comparison only regards the string content, not the view position.

@gilligan
Copy link
Contributor Author

gilligan commented May 7, 2020

@Kha thanks, fixed!

edolstra added 5 commits May 8, 2020
@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.

gilligan added 3 commits May 8, 2020
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
2 checks passed
2 checks passed
tests (ubuntu-18.04)
Details
tests (macos)
Details
@bhipple
Copy link
Contributor

bhipple commented May 8, 2020

This is phenomenal! Thanks guys!

@nixos-discourse
Copy link

nixos-discourse commented May 11, 2020

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
0 of 16 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.