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

GTest deprecated TYPED_TEST_CASE #3373

Closed
kunaltyagi opened this issue Sep 26, 2019 · 10 comments · Fixed by #3677
Closed

GTest deprecated TYPED_TEST_CASE #3373

kunaltyagi opened this issue Sep 26, 2019 · 10 comments · Fixed by #3677
Labels
good first issue Skills/areas of expertise needed to tackle the issue module: test

Comments

@kunaltyagi
Copy link
Member

kunaltyagi commented Sep 26, 2019

Warning is as follows:

pcl/test/common/test_geometry.cpp:49:1: warning: 'TypedTestCaseIsDeprecated' is deprecated: TYPED_TEST_CASE is deprecated, please use TYPED_TEST_SUITE [-Wdeprecated-declarations]
TYPED_TEST_CASE(XYZPointTypesTest, XYZPointTypes);
^

This occurs in many places throughout tests.

Your Environment

  • Operating System and version: Linux
  • Compiler: clang 8.0.1
  • PCL Version: master

The GTest used is from the master branch as per the "gtest" best-practices instead of OS installed version.

Expected Behavior

No warnings

Possible Solution

A simple sed might solve this

@kunaltyagi kunaltyagi added good first issue Skills/areas of expertise needed to tackle the issue module: test labels Sep 26, 2019
@taketwo
Copy link
Member

taketwo commented Sep 26, 2019

Which version of GTest is that? I don't get any warnings with 1.8.0. If we are to fix these, a solution that works with GTest 1.7.0 (default on Ubuntu 16.04) is needed.

@kunaltyagi
Copy link
Member Author

Which version of GTest is that?

I use master (simple git clone) and point CMake to the current default.

a solution that works with GTest 1.7.0 (default on Ubuntu 16.04) is needed

That... might be difficult. I did a grep and no search showed up that might suggest that TYPED_TEST_SUITE would work on 1.7.0

@jammm
Copy link
Contributor

jammm commented Oct 16, 2019

These warnings don't seem to appear when using GCC gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0

@kunaltyagi
Copy link
Member Author

kunaltyagi commented Oct 16, 2019

The warnings arise while using

  • googletest@HEAD (not system installed gtest which is usually present on Ubuntu)
  • clang (not GCC)

I reproduced them with the following details:

$ clang --version
clang version 9.0.0 (tags/RELEASE_900/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
$ cd googletest && git log -n1
commit ba513d2c9525a7c986c115ed5d603f2cf17c6016 (HEAD -> master, origin/master, origin/HEAD)
$ cd pcl && $ git log -n1
commit 5e418064c04976281f0b99712b181ad26f416368 (HEAD -> master, upstream/master, origin/master, origin/HEAD)
$ cd build && CC=clang CXX=clang++ \
cmake \
-DBUILD_global_tests=ON \
-DGTEST_SRC_DIR=${PREFIX}/googletest/googletest -DGTEST_INCLUDE_DIR=${PREFIX}/googletest/googletest/include/ \
-GNinja \
..
$ ninja test/common/test_common

Here, ${PREFIX} is the root dir for the source code

EDIT: Updated. Also, ninja isn't required, I just prefer it.
I'm reproducing with a more toned down cmake options.

@SunBlack
Copy link
Contributor

Just to mention: This is deprecated since GoogleTest 1.10.x.

@shrijitsingh99
Copy link
Contributor

shrijitsingh99 commented Feb 25, 2020

I was planning to put in a PR for this but I see that some work has already been done here #3419
So should I build upon that and submit a new PR since the original author has already put in some work so should I give credit?

@taketwo
Copy link
Member

taketwo commented Feb 25, 2020

  1. You are welcome to pick up and continue someone else's work if he/she lost interest or does not have time (this is the case here).
  2. Avoid duplicating work, especially if there exists almost finished PR (this is the case here).
  3. Add your commits on top of the existing branch. But you'll have to open a new PR because you don't have rights to push to the original author's fork.
  4. In the new PR refer to the old one and give credit to the original author, I think that's sufficient.

@kunaltyagi
Copy link
Member Author

  1. Add your commits on top of the existing branch..

You just needs to checkout to this pr, set a local branch (a nice to remember name) for it and set its upstream remote as the PR. No need to push to author's fork, right? I didn't have to push to Sergio's branch in order to modify his work. I might be wrong though

@shrijitsingh99
Copy link
Contributor

I don't think I can push directly to the PR.
Till I know GitHub only allow maintainers to make changes to the PR if granted permission

@kunaltyagi
Copy link
Member Author

Forgot I was a maintainer 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Skills/areas of expertise needed to tackle the issue module: test
Projects
None yet
5 participants