Migrate test suite to GoogleTest framework #119

davidtrevelyan · 2023-11-27T11:01:06Z

This PR does the following:

Introduces GoogleTest as the testing framework for RTNeural
Introduces two test targets: rtneural_test_unit and rtneural_test_functional
- Unit tests contain a small new example demonstrating unit testing patterns for Activation layers
- Functional tests are simply the old test header files, moved to the new target. Some old assertions have been simplified to make use of test assertions built into GoogleMock, and there have been a couple of very minor clarifying refactors in the test code. No new functional tests have been added.
Adds a new README file in the tests folder, explaining how to build and run the tests (or a subset of them!)
Updates test implementations to use #include <RTNeural/RTNeural.h>, to match the intended client code inclusion pattern

…amework

codecov-commenter · 2023-11-27T19:39:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1e7b570) 95.64% compared to head (5ddbdee) 95.59%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
- Coverage   95.64%   95.59%   -0.05%     
==========================================
  Files          58       58              
  Lines        3860     3794      -66     
==========================================
- Hits         3692     3627      -65     
+ Misses        168      167       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jatinchowdhury18

Thanks for the PR! The changes look good overall, I just had a couple of notes.

For the RTNeural header, I think using #include <RTNeural/RTNeural.h> in both the tests and client code is the way to go.
I think there's some kind of linker error happening with gmock on MSVC. I think this can be fixed by adding this to our test targets:

set_property(TARGET foo PROPERTY
  MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")

jatinchowdhury18 · 2023-11-27T19:48:30Z

cmake/Testing.cmake

+macro(rtneural_setup_testing)
+    include(CTest)
+    enable_testing()
+    add_custom_target(rtneural_test COMMAND ctest -C ${Configuration} --output-on-failure)


jatinchowdhury18 · 2023-11-27T19:49:33Z

cmake/Testing.cmake

+option(RTNEURAL_TEST_REPORTS "Output test reports to XML files" OFF)
+
+macro(rtneural_setup_testing)
+    include(CTest)


I was under the impression that CTest has to be included from the "top level" CMake script. But maybe this is okay because it's in a macro? (I've never used CMake macro's before)

I believe you're correct on both counts :)

A macro in CMake is somewhat analogous to a C preprocessor macro - after the arguments are substituted into the body, it's as if the implementation of the macro is just copy/pasted into the call site. This particular macro has no arguments, so it's pretty much just a straight copy/paste of the macro body! The result is as you suspect - CTest is indeed included at the top level CMake script here 👍

jatinchowdhury18 · 2023-11-27T20:33:51Z

tests/README.md

+1. unit tests, and
+2. functional tests.


This is great! I wonder if it might make sense to further split up the function tests into "layer" tests and "model" tests... at the moment we only have one model test, but I'm hoping to add more. Anyway, that's a change we can make later if we want to.

Glad you like it! On splitting the functional tests into tests for layers vs models, I'm not sure. I'd be happy to jump on a call to chat about it with you at some point, if you like! I would consider whether the layer tests are simple enough to be written as unit tests - they're usually simpler, easier to reason about, and less brittle. But every project has different needs and challenges, so there's no one-size-fits-all :)

Yeah, that's a good point. I'd like to do a bit of hacking myself just to get a sense of what might work, but yeah, no rush or anything, just putting some thoughts out there.

jatinchowdhury18 · 2023-11-27T20:35:27Z

RTNeural/CMakeLists.txt

-        ..
+        ./


I think I would prefer to leave the include path as #include <RTNeural/RTNeural.h>, just to avoid having a breaking change for end-users, and then we could update the tests to use that include path also?

For sure - sounds great. Totally agree on avoiding a breaking change. I'll update the tests and revert the include path!

This one's done, and I've updated the PR description to match!

davidtrevelyan · 2023-11-27T21:49:56Z

Thanks for the PR! The changes look good overall, I just had a couple of notes.

For the RTNeural header, I think using #include <RTNeural/RTNeural.h> in both the tests and client code is the way to go.

I think there's some kind of linker error happening with gmock on MSVC. I think this can be fixed by adding this to our test targets:
set_property(TARGET foo PROPERTY
  MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")

No problem - thanks for the review!

Regarding the include directories and include style, I've reverted the target property and have updated the tests to use the new style, and
regarding the gmock linker error - oops! Missed that one. I think the solution to this is in the GoogleTest README, so I'll try that first. If it doesn't work, let's try doing it manually as you suggest?

jatinchowdhury18 · 2023-11-27T21:54:30Z

Awesome, thanks for the update! Yeah, trying the CMake option provided by GoogleTest sounds good.

I think there's also a different include path thing happening in some of the examples, but otherwise we should be good to go :)

jatinchowdhury18 · 2023-11-28T00:59:13Z

Awesome, I'm happy to merge whenever you're ready, the only last thing would be to add yourself to the contributor's list if you'd like to.

davidtrevelyan · 2023-11-28T09:51:31Z

Nice one, thanks for the excellent review @jatinchowdhury18! I've updated the contributors list now and I'm ready for it to be merged too, assuming the CI passes 😁

davidtrevelyan added 9 commits November 22, 2023 00:12

Refactor tests to use googletest framework

8f802af

Add tests/README.md

742f529

Add coverage flags to unit tests

fad3a15

Update tests README

3c8b754

Update tests README

f963ca8

Simplify test expectation and update README

6d16a79

Switch GoogleTest to fetched dependency

9f1b342

Merge remote-tracking branch 'upstream/main' into refactor/testing-fr…

15cb73c

…amework

Update tests README

e5e4f70

jatinchowdhury18 approved these changes Nov 27, 2023

View reviewed changes

jatinchowdhury18 mentioned this pull request Nov 27, 2023

Implement grouped convolutions #116

Merged

davidtrevelyan added 2 commits November 27, 2023 21:23

Revert include directory and update tests

4ba22d0

Fix examples build

eeeb3ee

Set gtest to link to dynamic CRT on Windows

a2f0d76

davidtrevelyan added 2 commits November 27, 2023 22:00

Fix example include statement

0334d72

Revert to not compiling Conv2D model test when AVX enabled

1fb6bf1

Update contributors list

5ddbdee

jatinchowdhury18 merged commit 6a2b7b8 into jatinchowdhury18:main Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate test suite to GoogleTest framework #119

Migrate test suite to GoogleTest framework #119

davidtrevelyan commented Nov 27, 2023 •

edited

Loading

codecov-commenter commented Nov 27, 2023 •

edited

Loading

jatinchowdhury18 left a comment

jatinchowdhury18 Nov 27, 2023

jatinchowdhury18 Nov 27, 2023

davidtrevelyan Nov 27, 2023

jatinchowdhury18 Nov 27, 2023

davidtrevelyan Nov 27, 2023

jatinchowdhury18 Nov 27, 2023

jatinchowdhury18 Nov 27, 2023

davidtrevelyan Nov 27, 2023

davidtrevelyan Nov 27, 2023

davidtrevelyan commented Nov 27, 2023

jatinchowdhury18 commented Nov 27, 2023

jatinchowdhury18 commented Nov 28, 2023

davidtrevelyan commented Nov 28, 2023

Migrate test suite to GoogleTest framework #119

Migrate test suite to GoogleTest framework #119

Conversation

davidtrevelyan commented Nov 27, 2023 • edited Loading

codecov-commenter commented Nov 27, 2023 • edited Loading

Codecov Report

jatinchowdhury18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtrevelyan commented Nov 27, 2023

jatinchowdhury18 commented Nov 27, 2023

jatinchowdhury18 commented Nov 28, 2023

davidtrevelyan commented Nov 28, 2023

davidtrevelyan commented Nov 27, 2023 •

edited

Loading

codecov-commenter commented Nov 27, 2023 •

edited

Loading