Issues with googletest conversion and test writing documentation

[jessemapel]

This issue is for collecting problems with the googletest and test conversion documentation on the wiki:

https://github.com/USGS-Astrogeology/ISIS3/wiki/Writing-ISIS-Tests-Using-Gtest-and-Ctest

• After logs now report as pvgroups are added to pvl logs (fixes #4914) #4990 the section on how to handle logging needs to be updated
• The app conversion section should only cover how to convert to a callable and not further decompose the application as that is the direction we have ended up going
• The section on fixtures should cover the different fixture files created in Split Fixtures up and cleaned test headers #5007
• Document what part of the Application API can be used safely in converted apps, for example calls like SetOutputCube(param) that take a parameter name cannot be used because the UI must be passed in.
• Document supported OS for compiling
• Document contributing new test data for legacy makefile tests
• Document adding new data sets to data area
• Document app to callable conversion status
• Create process for external devs to indicate what apps would be helpful to have converted
• Document ISD generation for tests that require a camera model
• How should image data be handled in tests. What is the maximum size for including binary image files? Can they be downloaded dynamically?

[KrisBecker]

There is another open issue that is potentially related to this issue. #4864 describes several learning moments that might be good to add to the documentation.

One particularly subtle problem in the application is worth highlighting. You cannot call Process::SetOutputCube("TO") in your main as it will segfault. You must call the other method with the same name that takes a filename and cube attributes.

[jessemapel]

We should probably also deprecate all of those Application functions to indicate that people should not use them.

[KrisBecker]

I am in the process of migrating from MacOS 10.15 Catalina to MacOS 12 Monterey, which has been released for a year now. (NOTE: Mac support needs clarity.) In my first attempt to build ISIS on Monterey, it appears to fail on Googetest itself. This may be because the Googletest version ISIS is pinned to is 3-4 years old.

Note I tried both Xcode versions 13.4.1 and 14 and both failed. I may look into Xcode 12.3 which looks like that SDK is explicitly being referenced in the build.

(Sep220920) kbecker@zion3 build % sudo xcode-select -s /Applications/Xcode_13.4.1.app
(Sep220920) kbecker@zion3 build % c++ -v
2022-09-21 14:10:06.252 xcodebuild[20166:222073] Requested but did not find extension point with identifier Xcode.IDEKit.ExtensionSentinelHostApplications for extension Xcode.DebuggerFoundation.AppExtensionHosts.watchOS of plug-in com.apple.dt.IDEWatchSupportCore
2022-09-21 14:10:06.252 xcodebuild[20166:222073] Requested but did not find extension point with identifier Xcode.IDEKit.ExtensionPointIdentifierToBundleIdentifier for extension Xcode.DebuggerFoundation.AppExtensionToBundleIdentifierMap.watchOS of plug-in com.apple.dt.IDEWatchSupportCore
Apple clang version 13.1.6 (clang-1316.0.21.2.5)
Target: x86_64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Applications/Xcode_13.4.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
(Sep220920) kbecker@zion3 build % sudo xcode-select --install
xcode-select: note: install requested for command line developer tools
(Sep220920) kbecker@zion3 build % c++ -v
Apple clang version 13.1.6 (clang-1316.0.21.2.5)
Target: x86_64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Applications/Xcode_13.4.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
(Sep220920) kbecker@zion3 build % pwd
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/build
(Sep220920) kbecker@zion3 build % cd ..
(Sep220920) kbecker@zion3 ISIS3 % /bin/rm -rf build
(Sep220920) kbecker@zion3 ISIS3 % mkdir build
(Sep220920) kbecker@zion3 ISIS3 % cd build
(Sep220920) kbecker@zion3 build % cmake -DisisData=/opt/isis4/data -DisisTestData=/opt/isis4/testData -DJPK2KFLAG=OFF  -DCMAKE_BUILD_TYPE=RELEASE -GNinja  ../isis
-- The C compiler identification is AppleClang 13.1.6.13160021
-- The CXX compiler identification is AppleClang 13.1.6.13160021
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Applications/Xcode_13.4.1.app/Contents/Developer/Toolchain

...

Results in this immediate compile error:

(Sep220920) kbecker@zion3 build % ninja install
[192/3667] Building CXX object gtest/googlemock/CMakeFiles/gmock.dir/src/gmock-all.cc.o
FAILED: gtest/googlemock/CMakeFiles/gmock.dir/src/gmock-all.cc.o
/Applications/Xcode_13.4.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DGTEST_CREATE_SHARED_LIBRARY=1 -DISISBUILDDIR=\"/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/build\" -DISISROOT=\"/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/isis\" -Dgmock_EXPORTS -I/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include -I/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock -isystem /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googletest/include -isystem /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googletest -Wall -fPIC -std=c++14 -DISIS_LITTLE_ENDIAN=1 -Wno-unused-parameter -Wno-overloaded-virtual -Wno-strict-aliasing -DUSE_UNSTABLE_GEOS_CPP_API=1 -Wno-strict-overflow -DENABLEJP2K=OFF -Wall -fPIC -std=c++14 -DISIS_LITTLE_ENDIAN=1 -Wno-unused-parameter -Wno-overloaded-virtual -Wno-strict-aliasing -DUSE_UNSTABLE_GEOS_CPP_API=1 -Wno-strict-overflow -DENABLEJP2K=OFF -O3 -DNDEBUG -isysroot /Applications/Xcode_13.4.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk -fPIC -Wall -Wshadow -Werror -Wconversion -DGTEST_HAS_PTHREAD=1 -fexceptions -W -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wunused-parameter -Wcast-align -Wchar-subscripts -Winline -Wredundant-decls -std=c++11 -MD -MT gtest/googlemock/CMakeFiles/gmock.dir/src/gmock-all.cc.o -MF gtest/googlemock/CMakeFiles/gmock.dir/src/gmock-all.cc.o.d -o gtest/googlemock/CMakeFiles/gmock.dir/src/gmock-all.cc.o -c /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/src/gmock-all.cc
In file included from /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/src/gmock-all.cc:39:
In file included from /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock.h:59:
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-actions.h:455:3: error: definition of implicit copy constructor for 'PolymorphicAction<testing::internal::ReturnNullAction>' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  GTEST_DISALLOW_ASSIGN_(PolymorphicAction);
  ^
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googletest/include/gtest/internal/gtest-port.h:682:8: note: expanded from macro 'GTEST_DISALLOW_ASSIGN_'
  void operator=(type const &) = delete
       ^
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-actions.h:1011:10: note: in implicit copy constructor for 'testing::PolymorphicAction<testing::internal::ReturnNullAction>' first required here
  return MakePolymorphicAction(internal::ReturnNullAction());
         ^
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-actions.h:455:3: error: definition of implicit copy constructor for 'PolymorphicAction<testing::internal::ReturnVoidAction>' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  GTEST_DISALLOW_ASSIGN_(PolymorphicAction);
  ^
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googletest/include/gtest/internal/gtest-port.h:682:8: note: expanded from macro 'GTEST_DISALLOW_ASSIGN_'
  void operator=(type const &) = delete
       ^
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-actions.h:1016:10: note: in implicit copy constructor for 'testing::PolymorphicAction<testing::internal::ReturnVoidAction>' first required here
  return MakePolymorphicAction(internal::ReturnVoidAction());
         ^
In file included from /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/src/gmock-all.cc:39:
In file included from /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock.h:61:
In file included from /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-function-mocker.h:39:
In file included from /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-generated-function-mockers.h:47:
In file included from /Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-spec-builders.h:75:
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-matchers.h:1506:3: error: definition of implicit copy constructor for 'FloatingEqMatcher<double>' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  GTEST_DISALLOW_ASSIGN_(FloatingEqMatcher);
  ^
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googletest/include/gtest/internal/gtest-port.h:682:8: note: expanded from macro 'GTEST_DISALLOW_ASSIGN_'
  void operator=(type const &) = delete
       ^
/Users/kbecker/ISIS/IsisDev/Sep220920/ISIS3/gtest/googlemock/include/gmock/gmock-matchers.h:3632:10: note: in implicit copy constructor for 'testing::internal::FloatingEqMatcher<double>' first required here
  return internal::FloatingEqMatcher<double>(rhs, false);
         ^

[jessemapel]

Yeah we've started encountering this issue now that we are able to upgrade to a newer Mac OS version. We're working on figuring out what commit of gtest we should move to. The more recent commits fix this but appear to introduce other issues and are not compatible with older OS. We may need to move gtest from a static included dependency to a separate dependency per OS.

[KrisBecker]

I will just mention that the process by which external developers can update the ISIS test data area is not defined.

The other mention I wanted to make is the current ISIS test framework (i.e., Fixtures) can lead to merge conflicts. However, it appears you are aware of this issue.

[KrisBecker]

Can you also provide status on the app/unit conversion effort? How many are completed? How many remain? Are there plans to complete this work?

[jessemapel]

The fixture conflicts should be greatly reduced by splitting the massive file up into multiple files. It will still happen sometimes, but less often.

We don't have a concerted effort to complete the app conversion processes because there are so many apps and they are relatively low value. So, we're just doing it as needed for test improvements and other improvements. We should setup a process for external people to request apps to be prioritized for conversion. I know @victoronline asked about this too.

[jessemapel]

For updating legacy makefil test data, we are trying not to do that and instead convert the app the app to a callable. So, maybe that can be incorporated into conversion requests. That way, we're not updating test data that will eventually be removed.

[KrisBecker]

Please add guidelines/recommendations for data inclusion in tests.

There is understandable concern about data volumes in all aspects of the ISIS system. However, fabrication of data for tests purposes should be discouraged where real data should be used. (Is it possible to utilize on-demand download of, say, image data from external sources for tests in real time?)

In the work to convert the NEAR/MSI camera model (see #4887), I had to rely on you, @jessemapel, to produce ISD data for testing - something I had no knowledge of how/why to do it and the process is not documented in this context.

[jlaura]

However, fabrication of data for tests purposes should be discouraged where real data should be used.

Thanks for the comment. The project, during the major test conversion efforts, made the decision to use fabricated or cooked data to the greatest extent possible. This decision was made in part because of the test volume. The other, more significant issue, is that using fabricated data yields better unit tests. They allow testing a range of edge cases and better overall validation of the underlying algorithm implementations. Sure, they take more time to write, but they are better tests.

This is not universally applied as you will some some applications (such as ingestion) where cropped versions of the data are used.

[jessemapel]

There is a time and a place for real data. Things like calibration and photometric apps should use real data, but they should use very deliberately chosen small data sets. It's worth having something about when and where it is appropriate.

[github-actions]

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.