Skip to content

Set kErrorOnUninstantiatedParameterizedTest et al. to true#2683

Open
bcsgh wants to merge 18 commits intogoogle:mainfrom
bcsgh:bcs_pr
Open

Set kErrorOnUninstantiatedParameterizedTest et al. to true#2683
bcsgh wants to merge 18 commits intogoogle:mainfrom
bcsgh:bcs_pr

Conversation

@bcsgh
Copy link
Copy Markdown

@bcsgh bcsgh commented Jan 24, 2020

This will start reporting missing instantiations of parameterized tests as a failing test under the GoogleTestVerification test suite (they are currently reported, but just as a notice from a passing test).

NOTE: It is expected that this will surface existing issues in downstream consumers.

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@asoffer asoffer self-assigned this Jan 24, 2020
@asoffer
Copy link
Copy Markdown
Contributor

asoffer commented Jan 24, 2020

As discussed, this may take some time to fix internal users, but we do intend to accept this eventually.

@bcsgh
Copy link
Copy Markdown
Author

bcsgh commented Jan 24, 2020

Yup. That was what I was expecting. Thanks.

zhangxy988 pushed a commit that referenced this pull request Jan 24, 2020
Deleted an orphaned duplicate file and exclude another that shouldn't be part of :gtest_all_test.

This showed up while trying to debug the presubmit failure for: #2683

PiperOrigin-RevId: 291398123
@bcsgh
Copy link
Copy Markdown
Author

bcsgh commented Jan 25, 2020

BTW: seems this fails presubmit by catching an extra INSTANTIATE_TEST_SUITE_P in the tests. It seems the issue is a orphaned .cc file and another that an overly wide glob() is picking up:

The relevant bit of the log:

[----------] 2 tests from GoogleTestVerification
[ RUN ] GoogleTestVerification.UninstantiatedParamaterizedTestSuite
googletest/test/googletest-param-test2-test.cc:51: Failure

Paramaterized test suite ExternalInstantiationTest is instantiated via INSTANTIATE_TEST_SUITE_P, but no tests are defined via TEST_P . No test cases will run.

Ideally, INSTANTIATE_TEST_SUITE_P should only ever be invoked from code that always depend on code that provides TEST_P. Failing to do so is often an indication of dead code, e.g. the last TEST_P was removed but the rest got left behind.

To suppress this error for this test suite, insert the following line (in a non-header) in the namespace it is defined in:

GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(ExternalInstantiationTest);
Stack trace: ...

[ FAILED ] GoogleTestVerification.UninstantiatedParamaterizedTestSuite (1 ms)
[ RUN ] GoogleTestVerification.UninstantiatedParamaterizedTestSuite
googletest/test/googletest-param-test2-test.cc:60: Failure

Paramaterized test suite InstantiationInMultipleTranslationUnitsTest is instantiated via INSTANTIATE_TEST_SUITE_P, but no tests are defined via TEST_P . No test cases will run.

Ideally, INSTANTIATE_TEST_SUITE_P should only ever be invoked from code that always depend on code that provides TEST_P. Failing to do so is often an indication of dead code, e.g. the last TEST_P was removed but the rest got left behind.

To suppress this error for this test suite, insert the following line (in a non-header) in the namespace it is defined in:

GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(InstantiationInMultipleTranslationUnitsTest);
Stack trace: ...
[ FAILED ] GoogleTestVerification.UninstantiatedParamaterizedTestSuite (0 ms)

rogeeff pushed a commit that referenced this pull request Jan 31, 2020
Deleted an orphaned duplicate file and exclude another that shouldn't be part of :gtest_all_test.

This showed up while trying to debug the presubmit failure for: #2683

PiperOrigin-RevId: 291398123
suertreus pushed a commit that referenced this pull request Feb 5, 2020
Deleted an orphaned duplicate file and exclude another that shouldn't be part of :gtest_all_test.

This showed up while trying to debug the presubmit failure for: #2683

PiperOrigin-RevId: 291398123
suertreus pushed a commit that referenced this pull request Feb 6, 2020
Deleted an orphaned duplicate file and exclude another that shouldn't be part of :gtest_all_test.

This showed up while trying to debug the presubmit failure for: #2683

PiperOrigin-RevId: 291398123
suertreus pushed a commit that referenced this pull request Feb 7, 2020
Deleted an orphaned duplicate file and exclude another that shouldn't be part of :gtest_all_test.

This showed up while trying to debug the presubmit failure for: #2683

PiperOrigin-RevId: 291398123
Abseil Team and others added 17 commits February 7, 2020 13:33
Add missing explicit keyword for gmock_Impl constructor.

When switching to using GMOCK_PP in ACTION* macroses `explicit` keyword was
missed in gmock_Impl constructor causing ClangTidy warnings in ACTION_P macro.

PiperOrigin-RevId: 291159975
Deleted an orphaned duplicate file and exclude another that shouldn't be part of :gtest_all_test.

This showed up while trying to debug the presubmit failure for: google#2683

PiperOrigin-RevId: 291398123
Move part of functionality of Matcher* class to the base one. Reduce copypaste.

Make constructor and conversion operator of Matcher* class independent of pump.

PiperOrigin-RevId: 291405510
Adds missing `#define` guard around `TEST_F(...)`

PiperOrigin-RevId: 291703056
Create implementation macroses for matchers to move variadic parameters to the
end of parameters list.

To save backward compatibility, old macroses will be still taking `description`
parameter as the last one. But they will use INTERNAL macro that takes
`description` as the second parameter.

PiperOrigin-RevId: 291724469
Add includes for type_traits and utility to gmock-function-mocker.h: macros in the file require these headers.

PiperOrigin-RevId: 291782497
Add documentation for ASSERT_DEBUG_DEATH/EXPECT_DEBUG_DEATH

PiperOrigin-RevId: 292138974
Fix use of reserved names.
Minimize code duplication needed for explict-vs-nonexplicit constructor.

PiperOrigin-RevId: 292555014
Disable warning C4800 for Visual Studio 2019.

The compiler warning C4800 is disabled by default in Visual Studio 2019,
but it can be enabled on the command line. The only version of
Visual Studio that does not support warning C4800 is Visual Studio 2017.

PiperOrigin-RevId: 292624510
Fix std::move to std::forward where appropriate to support reference types.

PiperOrigin-RevId: 292923058
Pass method's parameters count to internal GMOCK_METHOD* macro.

This will help removing copypaste in every GMOCK_METHOD* macro in future.

PiperOrigin-RevId: 292932554
...text exposed to GitHub repo https://www.github.com/google/googletest

PiperOrigin-RevId: 293438092
Get rid of gmock-generated-function-mockers.h and
gmock-generated-function-mockers.h.pump.

Stop using pump for GMOCK_METHOD* macroses generation.

PiperOrigin-RevId: 293454519
Tag the function generated by MATCHER with GTEST_ATTRIBUTE_UNUSED_ to fix CI builds of gmock-matchers_test.cc vs. -Wunused-function.

See google#2697 for breakage.

PiperOrigin-RevId: 293669752
Get rid of gmock-generated-matchers.h and gmock-generated-matchers.h.pump.

Stop using pump for MATCHER* macroses generation.

PiperOrigin-RevId: 293878808
@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@bcsgh
Copy link
Copy Markdown
Author

bcsgh commented Apr 18, 2020

Bump? Any Progress on this?

@antoniovicente
Copy link
Copy Markdown

To allow gradual detection and fix: would it be possible to add a compile time option to enable this mode?

@bcsgh
Copy link
Copy Markdown
Author

bcsgh commented May 17, 2020

IIRC that was actually proposed (by me, internally to Google) at one point. I don't remember the details, but it got turned down. (I think it ended up with me reluctantly agreeing with the argument?)

@antoniovicente
Copy link
Copy Markdown

IIRC that was actually proposed (by me, internally to Google) at one point. I don't remember the details, but it got turned down. (I think it ended up with me reluctantly agreeing with the argument?)

So it goes. I was fortunate enough to be cc'ed on an google-internal report about some tests in an open-source project that I'm a part of which are not running. I'm sad that I can't protect the open source project from regressing once the relevant tests are fixed.

jmarantz pushed a commit to envoyproxy/envoy that referenced this pull request May 19, 2020
Commit Message: test: Fix missing instantiation of parameterized tests.
Additional Description: gtest silently skips parameterized that have no INSTANTIATE_TEST_SUITE_P associated with them as described at google/googletest#2683 . Thanks @yurykats for pointing me at these failing tests. Also, filed #11246 for redis tests that are not running for this same reason.
Risk Level: n/a
Testing: test-only changes
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Antonio Vicente <avd@google.com>
@yurykats
Copy link
Copy Markdown

yurykats commented Jun 11, 2020

It happened: ec94d9f

@mr-c
Copy link
Copy Markdown

mr-c commented Nov 2, 2020

Hello, has googletest switched to "live at HEAD" instead of regular releases? If so, has a refactoring tool been issued for this change, as per the policy ?

Is there a changelog or other announcement mechanism that developers should be subscribing to?

Debian packaged a recent git snapshot of googletest, and now other Debian packages that build and run googletest tests at packaging building time are broken :-(

@bcsgh
Copy link
Copy Markdown
Author

bcsgh commented Nov 2, 2020

Note, this is mostly a bunch of anecdotal observations, so take them with a suitable number of grains of salt.

tl;dr; I don't think it's practicable to automate a significant portion of the work needed to fix issues this surfaces. I would suggest the best course of action is to manually fix things. In the case where there isn't an underlying bug being masked by the missing test, this is easy. If there is such a bug, then that bug will likely be the majority of the work.


I did the original work for building and deploying this internal to Google. In doing that, specifically in fixing the existing cases, there are approximately three actions that were taken to fix things: 1) Delete the Test case/suite, 2) Add a GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST or 3) Add instantiations. And the split between them was not too far off equal.

As for automating things:

  • Inside Google, case 2 was automated using clang's RefactoringEngine (that may still be around, but I no longer have access), but that was mostly because of the quantity of cases to be handled rater than them being hard to handle. And this is actually the least preferred resolution is most case as it can easily paper over actual bugs.
  • Case 1 could also be easily automated in a similar way (and shouldn't be bulk applied, for similar reasons).
  • Case 3 is fundamentally impossible to automate. Empirically, even tests value parameterized on bool have a non-negligible number of cases that can't just be instantiated for both true/false. I tried automating that for the cases I had access to and a bunch of cases still needed manual intervention (e.g. one or the other cases may only be valid in particular build environments).

And even then, the decision of which of those cases to apply will always require semantic understanding of the project that tools will never have (or if they do get that, then I'm out of work). At best, you can apply case 2 and file bugs to go back end manual inspect them.

Furthermore, in my experience, a non-trivial number of cases where the fix was to add instantiations surfaced real bugs which, in a nutshell, was the original motivation for the whole project.

@bcsgh
Copy link
Copy Markdown
Author

bcsgh commented Feb 7, 2024

kErrorOnUninstantiatedParameterizedTest et al. should probably be removed at this point. They have been true for 3.5 years.

openvela-robot pushed a commit to open-vela/external_googletest that referenced this pull request Jul 23, 2025
Deleted an orphaned duplicate file and exclude another that shouldn't be part of :gtest_all_test.

This showed up while trying to debug the presubmit failure for: google/googletest#2683

PiperOrigin-RevId: 291398123
liujinye-sys pushed a commit to open-vela/external_googletest that referenced this pull request Dec 16, 2025
Deleted an orphaned duplicate file and exclude another that shouldn't be part of :gtest_all_test.

This showed up while trying to debug the presubmit failure for: google/googletest#2683

PiperOrigin-RevId: 291398123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants