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.

Sign up for GitHub

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

Jump to bottom

Remove gtest file from cpp library #9816

Merged
merged 3 commits into from
Mar 5, 2021
Merged

Remove gtest file from cpp library #9816

merged 3 commits into from
Mar 5, 2021

Conversation

zymap
Copy link
Member

@zymap zymap commented Mar 5, 2021 •
edited
Loading

Motivation

PR #9072 introduces the gtest file and a new license header.
Actually, we can avoid introducing the new license header by
adding the macro in our code.

@zymap
Fix the apache-rat check
b2dd30b 
---

**Motivation**

PR apache#9072 introduces the gtest file but does not exclude for apache-rat
check. So that causes the apache-rat check to fail.
@zymap zymap self-assigned this Mar 5, 2021
@merlimat
Copy link
Contributor

merlimat commented Mar 5, 2021

A couple of notes on #9072:

  1. That header file is just a tiny macro, we shouldn't need to import a file from gtest for that
  2. The macro is about tests and must not be exposed in public API

eolivelli
eolivelli approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

One curiosity:
Aren't we running apache-rat at every PR validation?

@zymap
Copy link
Member Author

zymap commented Mar 5, 2021

We are not running the apache-rat so I added it at PR #9815

@zymap
Copy link
Member Author

zymap commented Mar 5, 2021 •
edited
Loading

@merlimat @BewareMyPower Could you please take a look at this? I remove the gtest file and move the macro into the lib dir. Thanks

BewareMyPower
BewareMyPower requested changes Mar 5, 2021
View reviewed changes
pulsar-client-cpp/lib/TestUtil.h Outdated Show resolved Hide resolved
@BewareMyPower
Copy link
Contributor

Since the original gtest_prod.h has been removed, the pom.xml should remove following code as well.

 <exclude>pulsar-client-cpp/include/gtest/gtest_prod.h</exclude>

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Mar 5, 2021 •
edited
Loading

There're three places that included the gtest_prod.h before:

lib/UnAckedMessageTrackerEnabled.h:21:#include "gtest/gtest_prod.h"
lib/MultiTopicsConsumerImpl.h:21:#include "gtest/gtest_prod.h"
lib/PartitionedConsumerImpl.h:21:#include "gtest/gtest_prod.h"

You should replace it with #include "TestUtil.h"

@zymap
Copy link
Member Author

zymap commented Mar 5, 2021

@BewareMyPower I address your comments. Please take a look. thank you

BewareMyPower
BewareMyPower approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

LGTM

eolivelli
eolivelli approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM
can you please change the PR title and description.
We should cite that we are working on the CPP client tests.

@zymap zymap changed the title Fix the apache-rat check Remove gtest file from cpp library Mar 5, 2021
@merlimat merlimat added this to the 2.8.0 milestone Mar 5, 2021
@merlimat merlimat merged commit 0d5f070 into apache:master Mar 5, 2021
zymap added a commit that referenced this pull request Mar 6, 2021
@zymap
Remove gtest file from cpp library (#9816)
c568834 
* Fix the apache-rat check
---

**Motivation**

PR #9072 introduces the gtest file but does not exclude for apache-rat
check. So that causes the apache-rat check to fail.

* Move the test to the another file

* Address comments

(cherry picked from commit 0d5f070)
@zymap zymap added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Mar 6, 2021
mlyahmed pushed a commit to mlyahmed/pulsar that referenced this pull request Mar 7, 2021
@zymap @mlyahmed
Remove gtest file from cpp library (apache#9816)
ca296cd 
* Fix the apache-rat check
---

**Motivation**

PR apache#9072 introduces the gtest file but does not exclude for apache-rat
check. So that causes the apache-rat check to fail.

* Move the test to the another file

* Address comments
zymap added a commit to streamnative/pulsar-archived that referenced this pull request Mar 8, 2021
@zymap
Remove gtest file from cpp library (apache#9816)
92a0d72 
* Fix the apache-rat check
---

**Motivation**

PR apache#9072 introduces the gtest file but does not exclude for apache-rat
check. So that causes the apache-rat check to fail.

* Move the test to the another file

* Address comments

(cherry picked from commit 0d5f070)
(cherry picked from commit c568834)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@eolivelli eolivelli eolivelli approved these changes

@merlimat merlimat merlimat approved these changes

@sijie sijie Awaiting requested review from sijie

Assignees

@zymap zymap

Labels
area/build cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.1
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.
5 participants
@zymap @merlimat @BewareMyPower @eolivelli @codelipenghui