search

paleolimbot / s2geography

Simple features (ish) for s2geometry
Other
29 stars 3 forks source link

Noisy Tests Build #14

Closed pramsey closed 1 month ago

pramsey commented 7 months ago

When build tests is turned on, the following warnings are enabled, which mostly (as far as I can tell) warn about things in the S2, OpenSSL and absl headers.

https://github.com/paleolimbot/s2geography/blob/master/CMakeLists.txt#L176-L178

This makes for a noisy build which probably hides any issues that might be in s2geography. Is there a rationale to keep them?

paleolimbot commented 7 months ago

I think they were copied from a previous conda-forge project (maybe @benbovy set that up?)...I have no problem removing them.

I'm sure you've figured this out by now, but I had lofty ambitions to port the R tests ( https://github.com/r-spatial/s2/tree/main/tests/testthat ) to C++ after I split this bit of code off, but I think what is there now is non-existent.

pramsey commented 7 months ago

Yes, I see there's very few tests, and I have another branch with a few extra WKT tests... I kind of banged up against the limits of my knowledge WRT how precisely the gtests are supposed to be done. It seems really duplicative to have a separate gtest executable for every .cc file, but I am not sure what the expected gtest conventions are. Do you have any guidance?

paleolimbot commented 7 months ago

Writing tests is hard; reorganizing them is easy! Any tests you're willing to contribute are welcome.

benbovy commented 1 month ago

I think they were copied from a previous conda-forge project (maybe @benbovy set that up?)...I have no problem removing them.

Sorry for the late reply (better late than never!).

I agree this is annoying.

If you haven't figured out how to remove the 3rd-party warnings, s2geometry v0.11 has now an option S2_USE_SYSTEM_INCLUDES to silence the warnings in s2 headers. Not sure that applies to the s2 dependencies (absl, openssl), though. I think we could also use here some cmake workarounds and tweak some properties of the imported targets to avoid that noise.