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 updates #598

Merged
merged 4 commits into from
Apr 21, 2024
Merged

GTest updates #598

merged 4 commits into from
Apr 21, 2024

Conversation

bennibbelink
Copy link
Contributor

@bennibbelink bennibbelink commented Apr 20, 2024

This PR has minor updates for compatibility with cyclus PR #1738:

  • Requires that the compiler supports C++17. We already require this in Cyclus, looks like this change hasn't been made in cycamore yet. Gtest requires at least C++14.
  • Change instances of INSTANTIATE_TEST_CASE_P to INSTANTIATE_TEST_SUITE_P

Copy link

github-actions bot commented Apr 20, 2024

Build Status Report - 09f46f0 - 2024-04-20 18:15:24 -0500

Build FROM cyclus_20.04_apt/cyclus:latest
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_20.04_apt/cyclus:stable
  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️
Build FROM cyclus_20.04_conda/cyclus:latest
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_20.04_conda/cyclus:stable
  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️
Build FROM cyclus_22.04_apt/cyclus:latest
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_apt/cyclus:stable
  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️
Build FROM cyclus_22.04_conda/cyclus:latest
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_conda/cyclus:stable
  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️

@bennibbelink
Copy link
Contributor Author

bennibbelink commented Apr 20, 2024

Hmm okay looks like INSTANTIATE_TEST_SUITE_P is a breaking change until the newer GTest library is used. Based on my local testing this is how I propose we move forward with this:

  • PR Cycamore (Require C++17 standard in cycamore #599) to require C++17 - this should fix the cycamore build with the new GTest Library (We should see CI in cyclus #1738 pass once this is merged)
  • PR Cyclus #1738 with the updated GTest library
  • Once Cyclus #1738 is merged the changes to INSTANTIATE_TEST_SUITE_P succeed here

@bennibbelink bennibbelink requested a review from gonuke April 21, 2024 17:47
@gonuke
Copy link
Member

gonuke commented Apr 21, 2024

BTW - it looks like you reran the upstream/downstream tests but the hash didn't change and the timestamp you are using is from the hash. Thus the timestamp/hash info isn't necessarily helpful to know when the tests occurred.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it is passing as expected on up/downstream tests. Ready to merge - thanks @bennibbelink

@gonuke gonuke merged commit 129cce7 into cyclus:main Apr 21, 2024
10 checks passed
@bennibbelink bennibbelink deleted the c++-17 branch April 21, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants