search

DOI-USGS / ISIS3

Integrated Software for Imagers and Spectrometers v3. ISIS3 is a digital image processing software package to manipulate imagery collected by current and past NASA and International planetary missions.
https://isis.astrogeology.usgs.gov
Other
197 stars 166 forks source link

Issues with googletest conversion and test writing documentation #5029

Closed jessemapel closed 11 months ago

jessemapel commented 2 years ago

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

KrisBecker commented 1 year ago

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 commented 1 year ago

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

KrisBecker commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

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.