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.

Sign up for GitHub

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

Jump to bottom

Fix error so latest version of googletest can be used #954

Closed
wants to merge 7 commits into from
Closed

Fix error so latest version of googletest can be used #954

wants to merge 7 commits into from

Conversation

Matthew-Boyd
Copy link
Contributor

Fix error in googletest macro so that the latest version of googletest can be used.

@brtietz I think the installation instructions can be updated now to use the latest gtest version

Closes #806

@Matthew-Boyd Matthew-Boyd added this to the 2022.11.21 Patch 1 milestone Dec 13, 2022
@Matthew-Boyd Matthew-Boyd self-assigned this Dec 13, 2022
@Matthew-Boyd
Copy link
Contributor Author

@brtietz I think the errors here in the CI are because the older version of gtest is still being used, while these new changes are meant for compatibility with the latest version.

@brtietz
Copy link
Collaborator

brtietz commented Jan 4, 2023

I was able to build successfully on Windows. The errors on Mac and Linux look different now. Is it better to remove the copy & assign macro or update these additional tests?

In file included from /home/runner/work/ssc/ssc/googletest/googletest/include/gtest/gtest-message.h:55:0,
                 from /home/runner/work/ssc/ssc/googletest/googletest/include/gtest/gtest-assertion-result.h:46,
                 from /home/runner/work/ssc/ssc/googletest/googletest/include/gtest/gtest.h:59,
                 from /home/runner/work/ssc/ssc/ssc/test/ssc_test/cmod_etes_etes_test.cpp:34:
/home/runner/work/ssc/ssc/googletest/googletest/include/gtest/internal/gtest-port.h:696:23: error: return type specification for constructor invalid
   type(type const&) = delete;                 \
                       ^
/home/runner/work/ssc/ssc/ssc/test/shared_test/vs_google_test_explorer_namespace.h:39:7: note: in expansion of macro ‘GTEST_DISALLOW_COPY_AND_ASSIGN_’
   int GTEST_DISALLOW_COPY_AND_ASSIGN_(\
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/ssc/ssc/ssc/test/shared_test/vs_google_test_explorer_namespace.h:57:3: note: in expansion of macro ‘NAMESPACE_GTEST_TEST_’
   NAMESPACE_GTEST_TEST_(namespace_name, test_case_name, test_name,\
   ^~~~~~~~~~~~~~~~~~~~~
/home/runner/work/ssc/ssc/ssc/test/ssc_test/cmod_etes_etes_test.cpp:43:1: note: in expansion of macro ‘NAMESPACE_TEST’
 NAMESPACE_TEST(etes_etes_test, EtesEtesCmod, Default_NoFinancial)
 ^~~~~~~~~~~~~~
[ 83%] Building CXX object test/CMakeFiles/Test.dir/ssc_test/cmod_fuelcell_test.cpp.o
cd /home/runner/work/ssc/ssc/ssc/build/test && /usr/bin/g++  -I/home/runner/work/ssc/ssc/googletest/include -I/home/runner/work/ssc/ssc/googletest/googletest/include -I/home/runner/work/ssc/ssc/googletest/googletest/include/gtest -I/home/runner/work/ssc/ssc/ssc/test/. -I/home/runner/work/ssc/ssc/ssc/test/input_cases -I/home/runner/work/ssc/ssc/ssc/test/shared_test -I/home/runner/work/ssc/ssc/ssc/test/ssc_test -I/home/runner/work/ssc/ssc/ssc/test/tcs_test -I/home/runner/work/ssc/ssc/ssc/test/../ssc -I/home/runner/work/ssc/ssc/ssc/test/../tcs -I/home/runner/work/ssc/ssc/ssc/test/../solarpilot -I/home/runner/work/ssc/ssc/ssc/test/../shared -I/home/runner/work/ssc/ssc/ssc/test/../splinter -O3 -DNDEBUG -fPIE  -D__64BIT__ -D__UNIX__ -O3 -DNDEBUG -Wno-deprecated-declarations -std=gnu++11 -MD -MT test/CMakeFiles/Test.dir/ssc_test/cmod_fuelcell_test.cpp.o -MF CMakeFiles/Test.dir/ssc_test/cmod_fuelcell_test.cpp.o.d -o CMakeFiles/Test.dir/ssc_test/cmod_fuelcell_test.cpp.o -c /home/runner/work/ssc/ssc/ssc/test/ssc_test/cmod_fuelcell_test.cpp
In file included from /home/runner/work/ssc/ssc/googletest/googletest/include/gtest/gtest-message.h:55:0,
                 from /home/runner/work/ssc/ssc/googletest/googletest/include/gtest/gtest-assertion-result.h:46,
                 from /home/runner/work/ssc/ssc/googletest/googletest/include/gtest/gtest.h:59,
                 from /home/runner/work/ssc/ssc/ssc/test/ssc_test/cmod_etes_ptes_test.cpp:34:
/home/runner/work/ssc/ssc/googletest/googletest/include/gtest/internal/gtest-port.h:696:23: error: return type specification for constructor invalid
   type(type const&) = delete;                 \
                       ^
/home/runner/work/ssc/ssc/ssc/test/shared_test/vs_google_test_explorer_namespace.h:39:7: note: in expansion of macro ‘GTEST_DISALLOW_COPY_AND_ASSIGN_’
   int GTEST_DISALLOW_COPY_AND_ASSIGN_(\
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/ssc/ssc/ssc/test/shared_test/vs_google_test_explorer_namespace.h:57:3: note: in expansion of macro ‘NAMESPACE_GTEST_TEST_’
   NAMESPACE_GTEST_TEST_(namespace_name, test_case_name, test_name,\
   ^~~~~~~~~~~~~~~~~~~~~
/home/runner/work/ssc/ssc/ssc/test/ssc_test/cmod_etes_ptes_test.cpp:43:1: note: in expansion of macro ‘NAMESPACE_TEST’
 NAMESPACE_TEST(etes_ptes_test, EtesPtesCmod, Default_NoFinancial)
 ^~~~~~~~~~~~~~

@Matthew-Boyd
Copy link
Contributor Author

I don't think we should remove the macro as it looks to be needed for the intended functionality, which is to allow categorization of the tests in the Visual Studio Test Explorer. Part of my recent change was to add the 'int' return type on line 39 of vs_google_test_explorer_namespace.h . However, this seems to break the code on Mac and Linux.

@sjanzou Can I please take you up on your offer to test this on Mac and Linux? And maybe try different return types or selectively set the return type via something like:
#if defined(WIN32) || defined(_WIN32) || defined(WIN32) || defined(NT)

@sjanzou
Copy link
Collaborator

sjanzou commented Jan 18, 2023

I don't think we should remove the macro as it looks to be needed for the intended functionality, which is to allow categorization of the tests in the Visual Studio Test Explorer. Part of my recent change was to add the 'int' return type on line 39 of vs_google_test_explorer_namespace.h . However, this seems to break the code on Mac and Linux.

@sjanzou Can I please take you up on your offer to test this on Mac and Linux? And maybe try different return types or selectively set the return type via something like: #if defined(WIN32) || defined(_WIN32) || defined(WIN32) || defined(NT)

@Matthew-Boyd , thanks for the details - I am running locally on CentOS7 and MacOS 13 and will update here.

@sjanzou
Copy link
Collaborator

sjanzou commented Jan 19, 2023

I don't think we should remove the macro as it looks to be needed for the intended functionality, which is to allow categorization of the tests in the Visual Studio Test Explorer. Part of my recent change was to add the 'int' return type on line 39 of vs_google_test_explorer_namespace.h . However, this seems to break the code on Mac and Linux.

@sjanzou Can I please take you up on your offer to test this on Mac and Linux? And maybe try different return types or selectively set the return type via something like: #if defined(WIN32) || defined(_WIN32) || defined(WIN32) || defined(NT)

@Matthew-Boyd, the #if worked fine on MacOS with the "int" removed using the old googletest. However, for the new googletest, the gtest-port.h line 270 requires c++14
image

We will definitely not be able to support that when building on CentOS7.

So, I am not sure if there is a path forward if we need to support the Eagle HPC configuration and the latest googletest.

The latet commit on this branch updated all workflows and CMakelist.txt to C++14 for testing.

@Matthew-Boyd
Copy link
Contributor Author

@sjanzou Thanks a lot for looking into this and finding the limitation.

It looks like the options are to keep googletest pinned at that previous version or remove this categorization. I'm fine with either, and I may be the only one who cares about this feature. @brtietz , do you want to make the decision? If we're not keeping it pinned, I'll update the categorized tests so there's no errors.

image

@brtietz
Copy link
Collaborator

brtietz commented Jan 20, 2023

@Matthew-Boyd I put this on the meeting agenda for discussion. Some things I'm thinking about:

  1. Is anyone actually building the code on Eagle, or are they just using PySAM?
  2. Even if they are, are they running the tests? (is there a CMake flag we could use for the test file?)
  3. We can migrate away from CentOS7 when Kestrel comes online (tentatively July)

@Matthew-Boyd
Copy link
Contributor Author

Revisit in July '23 after we upgrade to C++14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brtietz brtietz Awaiting requested review from brtietz

@sjanzou sjanzou Awaiting requested review from sjanzou

Assignees

@Matthew-Boyd Matthew-Boyd

Labels
bug
Projects
None yet
Milestone
2022.11.21 Patch 1
Development

Successfully merging this pull request may close these issues.

Update tests to latest version of gtest
3 participants
@Matthew-Boyd @brtietz @sjanzou