JGCRI / hector

The Hector Simple Climate Model
http://jgcri.github.io/hector/
GNU General Public License v3.0
111 stars 47 forks source link

Bring test_dependency_finder.cpp up to date and re-enable it #654

Closed bpbond closed 1 year ago

bpbond commented 1 year ago

See #450

@pralitp Re-enabling this Hector test file was triggering the following error from inside the googletest framework itself:

Screen Shot 2022-11-03 at 7 02 25 AM

...that stemmed from lines in test_dependency_finder.cpp like this:

Screen Shot 2022-11-03 at 7 07 53 AM

As you can see from the diff, I removed the << "Ordering: " << ordering << endl end of the lines, after which things build and tests run fine. But I don't quite understand why. Any insight? Are these lines now giving false positives, or was this something cosmetic that was fine to remove? Thanks.

bpbond commented 1 year ago

@kdorheim #450 can be closed after merging #652 #653 #654

pralitp commented 1 year ago

Thanks, I'll take a look. My first guess is there is no operator<< being found for a std::vector. Which generally speaking there is none.. was gtest providing one? Did we used to provide one somewhere and it got moved around in such a way that it is now out of scope to be found for gtest (given the comments near the error it is much trickier than would otherwise seem).

bpbond commented 1 year ago

Thanks Pralit!

Did we used to provide one somewhere and it got moved around in such a way that it is now out of scope

Not that I know of.

bpbond commented 1 year ago

P.S. I guess the only thing that's being lost is the custom failure message, so not urgent.

pralitp commented 1 year ago

I just reverted removing the custom messages and it built and ran the tests just fine. However, I used the "HEAD" googletest as they recommended which might make a difference (or compiler: I used GCC 10.2)

@bpbond were you getting this locally or when enabled via GitHub Actions? If the later I can try again in Docker to try to get closer to what GH actions is doing.

bpbond commented 1 year ago

It ran fine for me locally; only on GA was there a problem.

Sorry, incorrect.

bpbond commented 1 year ago

@pralitp OK, it runs fine on GA. I will revert those changes, update my local googletest, and push. OK to merge after that?

Thanks!

pralitp commented 1 year ago

Great! One minor fix I was going to suggest but can obviously go elsewhere is a doc fix:

diff --git a/vignettes/articles/BuildHector.Rmd b/vignettes/articles/BuildHector.Rmd
index 03c9690..69e7bea 100644
--- a/vignettes/articles/BuildHector.Rmd
+++ b/vignettes/articles/BuildHector.Rmd
@@ -164,7 +164,7 @@ To run build and run the Hector unit tests, type `make testing`
 and then

-./src/Testing/hector-unit-tests
+./src/unit-testing/hector-unit-tests

 ## Xcode (Mac OS X)
bpbond commented 1 year ago

Skipped tests since just a single doc file modified in c3f28fd3