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.

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

Fix the broken master by the upgrade of GTest #133

Merged
merged 4 commits into from
Dec 1, 2022

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Dec 1, 2022

Motivation

The ubuntu:latest image became ubuntu:22.04 after actions/runner-images#6399. The GTest is no longer compatible with the current code so the CI failed.

Modifications

Pass int to TestWithParam and stick the image version to ubuntu-22.04 instead of ubuntu-latest.

@BewareMyPower BewareMyPower changed the title Fix gtest failure (WIP) Fix gtest failure (the master is broken) Dec 1, 2022
@erobot
Copy link
Contributor

erobot commented Dec 1, 2022

Maybe I have fixing the same problem. A fix is testing in erobot#4, have passed build but not complete ci yet.

@BewareMyPower
Copy link
Contributor Author

@erobot It seems to be a bug of gtest. I fixed it in another way (add an inline overload for const reference). Did you have any thought?

@BewareMyPower BewareMyPower changed the title (WIP) Fix gtest failure (the master is broken) Fix the broken master by the upgrade of GTest Dec 1, 2022
@BewareMyPower BewareMyPower self-assigned this Dec 1, 2022
@erobot
Copy link
Contributor

erobot commented Dec 1, 2022

@BewareMyPower I have a test that if change enum class KeyValueEncodingType to enum KeyValueEncodingType then build can succeed. I don't know the details but I guess maybe gcc-11 have something changed about Name lookup (or about Argument-dependent lookup?) for operator<< of enum class.

@BewareMyPower
Copy link
Contributor Author

Not sure if it's a bug for GCC 11 or GTest itself. Let's continue this fix because it doesn't change the public API.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM.

In another perspective, I'd prefer a fixed version of run-os instead of latest so that we explicit do the upgration on demand.

@BewareMyPower
Copy link
Contributor Author

I'd prefer a fixed version of run-os instead of latest so that we explicit do the upgration on demand.

Agreed. I will modify it in this PR.

@BewareMyPower
Copy link
Contributor Author

I opened an issue here: google/googletest#4079.

@BewareMyPower
Copy link
Contributor Author

Now the tests work for Ubuntu 22.04 (GCC 11) but it failed for Ubuntu 20.04 (GCC 9) in my local env.

/usr/include/gtest/gtest-printers.h:287:7: error: ambiguous overload for ‘operator<<’ (operand types are ‘std::ostream’ {aka ‘std::basic_ostream<char>’} and ‘const pulsar::KeyValueEncodingType’)
  287 |   *os << value;
      |   ~~~~^~~~~~~~

I decided to use another way to fix to avoid API changes.

@BewareMyPower BewareMyPower merged commit 91a97eb into apache:main Dec 1, 2022
@BewareMyPower BewareMyPower deleted the bewaremypower/test-ci branch December 1, 2022 07:32
@BewareMyPower BewareMyPower added this to the 3.2.0 milestone Feb 13, 2023
BewareMyPower added a commit that referenced this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release/3.1.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants