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

[extlibs/gtest] Update gtest & clean the CMakeLists.txt #604

Merged
merged 6 commits into from Mar 8, 2018

Conversation

2 participants
@damienmarchal
Copy link
Contributor

damienmarchal commented Mar 1, 2018

This shouldn't break anything and is a first step toward the slighgly more ambitious pr #601

CHANGELOG:

  • update gtest from their master (version 1.9: sha1 447d58b4ee8ea96b4757a5bb5f0b3be75af6c2a1)
  • update the CMakeLists so that they correctly export the needed -D to be used as a shared library
  • Fix the problem of missing GTEST_API_ operator<< gtest by implementing the function in TestMessageHandler_test instead of gtest.

NB: A better FIX for the GTEST_API_ operator<< could be to make submit PR to gtest but I have no more time left.


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Mar 1, 2018

[ci-build]

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Mar 2, 2018

@guparan Here it is the failure on windows are removed.
Do we go fast path ? [ci-build][with-scene-tests]

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Mar 2, 2018

Could you just specify the new GTest version in the description please?

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Mar 2, 2018

The current version we now have in extlibs 1.9...but actually the update didn't needed anychange so I assume that with the old gtest we had the fix would have work equally well.

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Mar 2, 2018

Yes... with the CImgPlugin fix you are doing the windows platform will be green :)

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Mar 2, 2018

Cool, green is my favorite color ;-)

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Mar 2, 2018

@hugtalbot @bcarrez can you make a quick review of this...so that we can work fast merge for this one.

@damienmarchal damienmarchal referenced this pull request Mar 2, 2018

Closed

[all] Refactor BaseTest & BaseSimulationTest #605

0 of 6 tasks complete
@guparan

This comment has been minimized.

Copy link
Member

guparan commented Mar 5, 2018

I just added a commit to clean the custom EXPECT_ATLEASE_ONE_NONFATAL_FAILURE macro. Tell me if I'm wrong but it would better be a macro expecting MORE than 1 failure to clearly distinct from EXPECT_NONFATAL_FAILURE (GTest one).

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Mar 5, 2018

Well I don't really see the cleaning there just renaming to a name I find more ambiguous because EXPECT_NONFATAL_FAILURES is much less 'distinct' fom EXPECT_NONFATAL_FAILURE than the previous one... but maybe I miss-understood the commit.

Anyway if you think this is better I don't really care as this is a macro to "test" the testing framework so of very limited usage.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Mar 6, 2018

The most important is just the if (results.size() <= 1) in HasFailures.
About the name, I started by fixing the typo "ATLEASE" and finnaly changed the whole thing but it's not very important we can go back to "ATLEAST" if you prefer :-)

EDIT: Actually we can't since the new behavior is to pass only if there is MORE than 1 failure.

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Mar 6, 2018

@guparan +for the typo and for the careful reading. Thanks.
With the new condition the macro is now equivalent to something like EXPECT_ATLEAST_TWO_FAILURES ? But as you can see in the dashboard this is not what the tests are expecting as there is now one new test failure reported in the dashboard.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Mar 6, 2018

Ok I didn't take the time to run the tests myself so I wanted to see the Dashboard. Is it actually possible to have more than 1 failure in a test ? Both "ATLEAST_ONE" and "ATLEAST_TWO" seem useless there.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Mar 6, 2018

Just tried it and using EXPECT_NONFATAL_FAILURE works well on Windows.
Why did you create EXPECT_ATLEAST_TWO_FAILURES in the first place @damienmarchal ?

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Mar 6, 2018

Because defaultTestBehavior was supposed to fail three time and not one (this is not working with EXPECT_NONFATAL_FAILURE). The fact it actually fails only once has been changed later by changing the default behavior in BaseTest. The rational for this change was that for a transitional period of time it was decided to avoid warnings messages to generate a test failure so that the dashboard is less "red" and the tests was not refactor since this decision.

EDIT: I didn't made EXPECT_ATLEAST_TWO_FAILURES...only ONE.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Mar 6, 2018

Not sure to understand. Shall we safely remove this custom macro (ONE and TWO versions) or do we want to keep it for a transitional period?
If we keep it, I have to redo my commit.

@guparan guparan force-pushed the SofaDefrost:fixGtestOnly branch from dd894ac to a18f1c5 Mar 7, 2018

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Mar 7, 2018

I just replaced my commit by a simple typo fix.
Thank you for the clear explanations @damienmarchal

@guparan guparan merged commit 2834007 into sofa-framework:master Mar 8, 2018

6 checks passed

Dashboard Builds triggered.
Details
Scene tests Triggered in latest build.
Details
centos_clang-3.4_options OK (tests ignored, see details)
Details
mac_clang-3.4_options OK (tests ignored, see details)
Details
ubuntu_gcc-5.4_options OK (tests ignored, see details)
Details
windows7_VS-2015_options_amd64 OK (tests ignored, see details)
Details

@guparan guparan added this to the v18.06 milestone Apr 5, 2018

@damienmarchal damienmarchal deleted the SofaDefrost:fixGtestOnly branch Dec 11, 2018

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