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

WIP: Google Test Framework for Unit Testing BOINC #2443

Closed
wants to merge 17 commits into from

Conversation

Uplinger
Copy link
Contributor

@Uplinger Uplinger commented Apr 2, 2018

Greetings,

I would like to get some feedback from the community about this approach to doing Unit Testing against the BOINC source code.

I have documented the design and initial thoughts here: https://github.com/Uplinger/boinc/wiki/BOINC-Google-Test-Design

The Code in this pull request reflect what is mentioned in the wiki page and gives people a chance to test it out for themselves.

Thanks,
-Keith Uplinger

JuhaSointusalo and others added 10 commits March 30, 2018 11:23
The client shows what kind of operating system it has detected in startup log message and elsewhere. This is confusing and not helping in troubleshooting when people expect these messages to describe the client itself.

Change the messages to use client's build platform instead.

Startup log message, output of --version and user agent string should be safe to change without breaking anything. The client still uses OS platform in various places but changing these would break stuff:

- Scheduler request XML. Scheduler expects scheduler_request.platform_name to be OS primary platform.
- client_state.xml. Client writes OS primary platform and alt platforms separately to client_state.xml. The client reads primary platform only to see if it has changed. The client doesn't read alt platforms but some other program might and depends on the platforms being as they are now. Not changing this means the client won't be able to detect when its own platform has changed. OS platform changes are still detected.
- get_state GUI RPC. The client writes primary platform to client_state.platform_name and all platforms, including primary, to client_state.platform. BOINC Manager only reads client_state.platform. As such, client_state.platform_name could be changed to client's build platform but some 3rd party manager might depend on it being OS primary platform.
- time_stats_log. The client writes platform info there. Unclear who uses that information.

Closes BOINC#2386.
…directory. test_url added with a simple check
Allow projects to show (non-DB) content even if stop_web is present
(e.g. description of the project on front page).
stop_web really means "the DB is offline".
client: show build not OS platform in messages
web: don't check for stop_web in page_head().
@Uplinger
Copy link
Contributor Author

Uplinger commented Apr 5, 2018

@AenBleidd @brevilo Can you guys take a look at this proposal as I think both of you have shown interest in adding automated testing to the BOINC framework.

@brevilo
Copy link
Contributor

brevilo commented Apr 6, 2018

Hopefully next week...

@AenBleidd
Copy link
Member

I'll try to take a look this weekend, Maybe also will prepare some changes to Travis to run these tests.

@ChristianBeer
Copy link
Member

I briefly looked at the implementation and it seems sound. I also looked into integrating this with travis which seems trivial although you should make the googletest install path configurable so we can put it in the travis cache under 3rdParty/buildCache/linux treating it as a 3rdParty build dependency.

Getting coverage reports seems to be a bit more difficult as this involves running the compiled boinc binaries in the VM (I think). I haven't figured out how that works. A good service to use would be https://coveralls.io/ which has a github and Travis-CI integration.

@ChristianBeer
Copy link
Member

Follow up after I tested this locally in no particular order :

  • add --coverage to CXXFLAGS in all gtest Makefiles
  • add - if [[ "${TRAVIS_OS_NAME}" == "linux" ]]; then ( pip install --user cpp-coveralls ) fi to before_install in .travis.yml
  • install googletest via script to 3rdParty/buildCache/linux (currently testing if possible)
  • get PMC permission to create a coveralls account for BOINC
  • add something like coveralls --exclude gtest --gcov-options '\-lp' to after_success to upload coverage reports to coveralls

This is not intended to go into main source repo!
@ChristianBeer
Copy link
Member

I already did part of the above steps in a (hasty) commit and will open a PR to see if it works.

This is needed becaue of the way caching works. Needs to be improved since it builds wxWidgets when it is not needed.
@ChristianBeer
Copy link
Member

Final note: travis integration is working but needs some final adjustements to keep the cache small. See #2457

@ChristianBeer
Copy link
Member

I did some more testing and found a way to upload coverage reports to https://codecov.io.

@Uplinger
Copy link
Contributor Author

Uplinger commented Apr 8, 2018

Christian, thanks for taking a look at this. I haven't had a chance to view it this weekend. But I will tomorrow. Thanks again.

@Uplinger
Copy link
Contributor Author

Does anyone have experience turning the example Makefiles.gtest to actually work with configure that BOINC uses for all the other folders? I was hoping someone with more experience in that would be able to help with that part.

@Uplinger
Copy link
Contributor Author

@ChristianBeer Would you consider the code coverage part of your comments to be a secondary or follow up to the google testing framework? Reason I ask is that I would like to get the google testing going first without adding additional requirements like additional tools. Thoughts?

Thanks,
-Keith

@ChristianBeer
Copy link
Member

As it turned out the code coverage part didn't involve additional requirements other than the codecov.io account. Integrating code coverage consists of only two changes. Adding the --coverage flag to the BOINC makefiles and the extra script that creates and uploads the coverage reports. I can add this as a follow up.

Converting the fixed Makefiles to autogenerated makefiles is not that difficult I think. The tricky part is to detect the presence of the googletest library and finding the needed include path. If that fails we could place a preconfigured Makefile in gtest and it's subdirectories and add it to the recursion of the root Makefile. This is currently done in the samples directory to build for example condor or example_app. In the past I tried to get help with the autotools scripts but ended up doing the work on my own through trial and error.

@ChristianBeer
Copy link
Member

@Uplinger So what's the plan now?

  • Create Makefile.am files in gtest and all subdirs that mimic yours?
  • Clean and Rename your makefiles and call them from the Makefile in src root? (Would be easiest)
  • In General: Do we want to have one unitTests executable or one per subdirectory (gtest/lib and gtest/sched at the moment)
  • Are you ok with using a fixed path to the googletest library? If not we would need to add it to configure and pass it along.

My personal opinion is to go with the second option and use a fixed path to googletest. Dealing with automake and autoconf is a fight that might not be worth fighting. In the long term I see BOINC moving to cmake so I rather don't want to put much effort into expanding the current autotools build files.

@brevilo
Copy link
Contributor

brevilo commented Apr 16, 2018

Integrating code coverage consists of only two changes. Adding the --coverage flag to the BOINC makefiles and the extra script that creates and uploads the coverage reports.

Please ensure that any interaction with 3rd-party services uses an opt-in process. We'd like to be able to build and test BOINC without 3rd-party involvement or even an internet connection.

Thanks!

@brevilo
Copy link
Contributor

brevilo commented Apr 16, 2018

I read the proposal and I like it (well, standard unit testing, right?). My only comment from the one above is that you shouldn't make any assumption on where googletest lives, if possible. Ideally that would be configurable.

@ChristianBeer
Copy link
Member

Please ensure that any interaction with 3rd-party services uses an opt-in process. We'd like to be able to build and test BOINC without 3rd-party involvement or even an internet connection.

The generation and upload of coverage reports only applies to the Travis CI build. Users building and executing the test cases at home will not not be bothered with this. The only requirement is downloading the googletest library to the local system before building.

@ChristianBeer
Copy link
Member

I'll try to find some time this weekend to look into changing the autotools files to support ./configure --with-gtest= (similar to what is there for wxWidgets) and probably make check or make test

@ChristianBeer
Copy link
Member

I gave it a try and ended up with this branch: https://github.com/BOINC/boinc/commits/cb_gtest_framework which can build, install and detect googletest. It uses some outdated automake scripts that still seem to work.

The last thing to do is write some new Makefile.am that do the same as @Uplinger 's Makefile.gtest while using the GTEST_* variables in Makefile.incl. Maybe someone with a better grasp of automake can take a look at this?

What about using cmake for the unit testing stuff and have a kind of hybrid setup?

@lfield
Copy link
Contributor

lfield commented Apr 11, 2019

What is that status of this PR? There has been no progress for nearly one year.

@ChristianBeer
Copy link
Member

This is the first version of #2457 so it can be closed and the other should be reopened.

This pull request was closed.
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.

9 participants