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

Initial unit tests based on GoogleTest and GoogleMock #794

Closed
uweseimet opened this issue Aug 19, 2022 · 9 comments · Fixed by #802
Closed

Initial unit tests based on GoogleTest and GoogleMock #794

uweseimet opened this issue Aug 19, 2022 · 9 comments · Fixed by #802
Assignees

Comments

@uweseimet
Copy link
Contributor

uweseimet commented Aug 19, 2022

@akuker @rdmark The feature_google_test branch contains some initial unit tests based on GoogleTest, which I would like to be commented on before more time is spent on them. The relevant changes only affect the Makefile and add one additional source file in the new "test" folder.
Some notes:

  • There does not seem to be a pre-compiled GoogleTest for Raspberry Pi OS. I don't see an issue with that because as long as the unit tests are hardware-independent (otherwise they might rather be classified as integration tests) they run on any platform. Currently I run them on a regular Linux PC, which is a better development environment than the Pi anyway.
  • The tests are built and run with the "make test" target. The default "make" target tdoes not build them, so there are no compile-time issues (see above) on the Pi, and compiling on the Pi does not take longer than before.
  • In order to test not just a small part of the code but to have a substantial amount of tests, a lot of refactoring of the existing code may be required. The current code still lacks the required granularity and encapsulation. But it is normal the adding tests has an influence on your code structure, and you have to start somewhere ;-).
  • Restructure project folders to better accommodate (C++) unit tests #455 may have to be revisited at some point in time, but only if there are ever enough tests to justify spending time on Restructure project folders to better accommodate (C++) unit tests #455. There are already too many tickets that were started but are now in a rather undefined/unfinished state ...
>make test
./bin/fullspec/rascsi_test
Running main() from /var/tmp/portage/dev-cpp/gtest-1.11.0/work/googletest-release-1.11.0/googletest/src/gtest_main.cc
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from ModePagesTest
[ RUN      ] ModePagesTest.SCSIHD_AddModePages
[       OK ] ModePagesTest.SCSIHD_AddModePages (0 ms)
[ RUN      ] ModePagesTest.ModePageDevice_AddModePages
[       OK ] ModePagesTest.ModePageDevice_AddModePages (0 ms)
[----------] 2 tests from ModePagesTest (0 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 2 tests.
@uweseimet uweseimet added the enhancement New feature or request label Aug 19, 2022
@uweseimet uweseimet changed the title Initial unit tests based on GoogleTest Initial unit tests based on GoogleTest and GoogleMock Aug 20, 2022
@uweseimet uweseimet linked a pull request Aug 22, 2022 that will close this issue
@rdmark
Copy link
Member

rdmark commented Aug 23, 2022

@uweseimet It doesn't seem like x86 Debian comes with pre-compiled GoogleTest libraries either. The googletest and google-mock packages simply install source code into /usr/src/googletest and then expects you to include the code to build with your C++ project.

I tried pulling in the headers with the -I parameter but the process fails at linking because I don't think compiled libraries are available.

CPLUS_INCLUDE_PATH+="/usr/src/googletest/googletest/include:/usr/src/googletest/googlemock/include" make test

What Linux distro are you using for development?

@uweseimet
Copy link
Contributor Author

uweseimet commented Aug 23, 2022

@rdmark I am using Gentoo. But even if not all Linux distributions out of the box support GoogleTest, this should not block the PR because the tests are not compiled by default anyway, i.e. if they do not compile they do not break anything.
On most distributions https://www.eriksmistad.no/getting-started-with-google-test-on-ubuntu/ should work.

@rdmark
Copy link
Member

rdmark commented Aug 23, 2022

Thanks for this guide. Going through this, and then repeating the same for libgmock* got the tests to link. Confirmed working on Debian Sid bleeding edge. :)

sid

@rdmark
Copy link
Member

rdmark commented Aug 24, 2022

@uweseimet have you thought about tooling for measuring code coverage, e.g. line coverage metrics?

@uweseimet
Copy link
Contributor Author

uweseimet commented Aug 24, 2022

@rdmark I don't think I will spend time on this soon. Unfortunately everything related to C++ appears to be more or less a one-man-show, which is not so good ... If somebody was adding a ticket for code coverage she/he should also be prepared to work on this ticket.
Saying this, I think we should be more consequent in rejecting tickets. There are feature tickets that require major efforts (not only in C++ coding), and those who created the tickets for new features are not willing to work on them, or lack the knowledge to work on them. Such tickets should be rejected, because the project is obviously lacking resources.

@rdmark
Copy link
Member

rdmark commented Aug 25, 2022

@rdmark I don't think I will spend time on this soon. Unfortunately everything related to C++ appears to be more or less a one-man-show, which is not so good ... If somebody was adding a ticket for code coverage she/he should also be prepared to work on this ticket. Saying this, I think we should be more consequent in rejecting tickets. There are feature tickets that require major efforts (not only in C++ coding), and those who created the tickets for new features are not willing to work on them, or lack the knowledge to work on them. Such tickets should be rejected, because the project is obviously lacking resources.

In my opinion there is value to having a backlog of improvements and new features, even if there isn't anyone who can immediately work on them. It helps show the community a future potential roadmap, as well as serve as starter tickets for potential code contributors. That said, I'm not opposed to closing out lower importance improvement ideas to reduce the clutter.

Now, I'd be interested in investigating code coverage tooling myself. I asked just because I was curious if you already had something in mind.

@uweseimet
Copy link
Contributor Author

uweseimet commented Aug 26, 2022

@rdmark Actually, if I were adding tickets for all potential improvements I can think of there would be a flood of new tickets ;-). We could add tickets for any SCSI device type not yet implemented. In theory these kind of tickets would be legitimate, but in practice ...
The more tickets we have, the harder it gets to keep the overview. If more and more tickets are added and not resolved, this is an indicator that there is a problem with a project. Regarding starter tickets: There are several of them (labelled accordingly) already.
A backlog or roadmap requires prioritizing tickets and spending more time on project managment ;-). If you have tickets dealing with support for new hardware, for instance, the community may initially like this. But when months later nothing has happend it looks like there are a lot of ambitions, but nothing gets done. Just my two cents worth.

@uweseimet
Copy link
Contributor Author

uweseimet commented Aug 26, 2022

@rdmark I have not yet commented on code coverage tools. I have not investigated this. As long is there is no substantial amount of unit tests, collecting code coverage metrics is not that useful. In addition having the existing (2 years ago) GitHub actions automation ticket resolved would be helpful in this context, in order to have automated building, testing and collecting metrics.

Having a decent number of (C++) unit tests may be out of scope for this project anyway. Who is going to write and maintain these tests and refactor the legacy code to become testable? This is not necessarily something you can do on-the-fly. Look at the SASI removal and the related changes and refactorings which affected the SCSI code. If there had been unit tests for the SCSI code, updating these tests would have required quite some time.
Nevertheless unit tests are a good thing, of course. For rascsi in particular, because it is likely that some of the platforms in the compatibility list do not work anymore. A developer cannot test them all, and there do not seem to be many users who test their hardware with the develop branch, often not even with recent releases.

Regarding code coverage, which percentage would you want to achieve? 70-80% is a typical goal I think. Once I wanted to learn whether you could reach 100% in some of the Java projects I was involved with. I was surprised that this was actually possible, but how well this works depends on the nature of the project. Multithreading is not helpful here ;-).

@uweseimet
Copy link
Contributor Author

@rdmark Bear with me for being that talkative, but I just remembered that I had a look at how to use SonarQube for C++ some time ago, because I also use it for other languages. Besides code coverage metrics SonarQube provides a lot of additional (code qualitiy) metrics.
The free SonaQube edition does not support C++ code analysis. For open source projects one can use https://sonarcloud.io/explore/projects, but you would still need the C++ code scanner in your build environment, and the scanner does not appear to be free.

@uweseimet uweseimet removed the enhancement New feature or request label Aug 27, 2022
@uweseimet uweseimet self-assigned this Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants