Skip to content

Update example #3

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

Closed
wants to merge 4 commits into from
Closed

Update example #3

wants to merge 4 commits into from

Conversation

Lecrapouille
Copy link

No description provided.

@bastianhjaeger
Copy link
Owner

Thanks for your contribution!

I added a PR with 2 additional commits for fixing the MAcOS build issue here
#4

Note:

  • You can see in the install step Download, configure and install gtest where the libs are installed to:
-- Installing: /usr/local/lib/libgtest.a
-- Installing: /usr/local/lib/libgtest_main.a
-- Installing: /usr/local/lib/pkgconfig/gtest.pc
-- Installing: /usr/local/lib/pkgconfig/gtest_main.pc

Hence I added this path to the cmake. On linux this path is used by default. On MacOS apparently not.

I will leave some more comments next on the code.

Copy link
Owner

@bastianhjaeger bastianhjaeger left a comment

Choose a reason for hiding this comment

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

Thanks! Looking forward for this.

@@ -1,4 +1,4 @@
#include <github_actions_gtest_example/github_actions_gtest_example.h>
#include "github_actions_gtest_example/github_actions_gtest_example.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert. Not part of the PR issue.

Copy link
Author

@Lecrapouille Lecrapouille Mar 2, 2022

Choose a reason for hiding this comment

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

Yes not part of the PR, you're right but also no because do you really want searching your own include in system paths first ? This makes sense for library for example. Have you already reverted on your patch ?

src/info.cpp Outdated
@@ -1,4 +1,4 @@
#include <github_actions_gtest_example/github_actions_gtest_example.h>
#include "github_actions_gtest_example/github_actions_gtest_example.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert. Not part of the PR issue.

@@ -1,5 +1,5 @@
#include <github_actions_gtest_example/github_actions_gtest_example.h>

#include "github_actions_gtest_example/github_actions_gtest_example.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert. Not part of the PR issue.

@@ -1,3 +1,4 @@
#include <gmock/gmock.h>
Copy link
Owner

Choose a reason for hiding this comment

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

Why bringing in gmock everywhere? Seems not to be needed, right?
I probably do not mind, but I am curious.

Copy link
Author

@Lecrapouille Lecrapouille Mar 2, 2022

Choose a reason for hiding this comment

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

ok. I did not reaaly look at your gtest code. My main looks like:

#include <gtest/gtest.h>
// #include <gmock/gmock.h>

int main(int argc, char *argv[])
{
    // The following line must be executed to initialize Google Mock
    // (and Google Test) before running the tests.
    ::testing::InitGoogleMock(&argc, argv);

    return RUN_ALL_TESTS();
}

@Lecrapouille
Copy link
Author

Lecrapouille commented Mar 2, 2022

@bastianhjaeger Hence I added this path to the cmake. Good news, I spent hours not finding how to set this information, I'm not feeling comfortable with CMake and in addition order is important (I definitively hate CMake). So I cleaned and remade the pull request.

This pull request was closed.
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