You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/12 18:59:16 UTC
[GitHub] [geode-native] moleske opened a new pull request #765: Allow ipv6 test to be run
moleske opened a new pull request #765:
URL: https://github.com/apache/geode-native/pull/765
- Add ifdef for enable/disable ipv6 test
- Disable unit test when ipv6 is on
- Add CI job to use WITH_IPV6=ON
Authored-by: M. Oleske <mi...@oleske.engineer>
I'd like to be able to run the BasicIPv6Test in my pipeline I keep for geode native. The recent changes to get a concourse ci marked the BasicIPv6Test as disabled. The test works fine when setting the WITH_IPV6 variable. This change enables the test when the flag is set and disables when not. When testing, I noticed all tests work with ipv6 except for the unit test QueueConnectionRequestTest, so went ahead and did a similar enable/disable. I don't know if I went the best way about doing this so wanted some feedback/collaboration since I feel weird enabling/disabling tests.
I also added a change to ci/base/base.yml so that the pipeline could run the tests with ipv6. It only works on linux at the moment, so I chose ubuntu. I think it is valuable to run, but don't know if it should block releases. I haven't tested the concourse change, I would need help on that
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on pull request #765: GEODE-DIDN'TMAKEASTORYYET: Allow ipv6 test to be run
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #765:
URL: https://github.com/apache/geode-native/pull/765#issuecomment-797785266
> I say fix the test so it can run everywhere and make IPv6 on by default or use the `gtest` flags to run this test despite being disabled.
I will look to properly change gtest flags for the tests. The problem I get running this on windows is that the geode server won't start up https://github.com/moleske/geode-native/runs/2069274591#step:11:19
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #765: GEODE-DIDN'TMAKEASTORYYET: Allow ipv6 test to be run
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #765:
URL: https://github.com/apache/geode-native/pull/765#issuecomment-797804038
> I will look to properly change gtest flags for the tests. The problem I get running this on windows is that the geode server won't start up https://github.com/moleske/geode-native/runs/2069274591#step:11:19
It is that `ip6-localhost` address, which isn't a standard name. The standard is `localhost` for both IPv4 and IPv6 but since we have no method to tell the library to prefer one address over another it typically picks up IPv4. Try `::1` explicitly. I tried this in the past and `ACE` barfed but we replaced ACE sockets with Boos ASIO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on a change in pull request #765: GEODE-DIDN'TMAKEASTORYYET: Allow ipv6 test to be run
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #765:
URL: https://github.com/apache/geode-native/pull/765#discussion_r593477709
##########
File path: cppcache/test/CMakeLists.txt
##########
@@ -46,7 +50,7 @@ add_executable(apache-geode_unittests
LRUQueueTest.cpp
PdxInstanceImplTest.cpp
PdxTypeTest.cpp
- QueueConnectionRequestTest.cpp
+ ${ENABLE_QUEUECONNECTIONREQUESTTEST}
Review comment:
This test I didn't conditionally add the disable flag cause I got confused on how to do it and it wasn't working. That would probably be better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #765: GEODE-DIDN'TMAKEASTORYYET: Allow ipv6 test to be run
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #765:
URL: https://github.com/apache/geode-native/pull/765#discussion_r593473980
##########
File path: cppcache/test/CMakeLists.txt
##########
@@ -46,7 +50,7 @@ add_executable(apache-geode_unittests
LRUQueueTest.cpp
PdxInstanceImplTest.cpp
PdxTypeTest.cpp
- QueueConnectionRequestTest.cpp
+ ${ENABLE_QUEUECONNECTIONREQUESTTEST}
Review comment:
Why conditionally compile it here when the test itself conditionally adds the disabled flag?
##########
File path: ci/base/base.yml
##########
@@ -96,4 +96,11 @@ builds:
image_family: build-ubuntu-20-04
source_image_family: ubuntu-2004-lts
+ - _: #@ template.replace(new_build("ubuntu-20.04-ipv6"))
+ image_family: build-ubuntu-20-04
+ source_image_family: ubuntu-2004-lts
+ #@yaml/map-key-override
+ params:
+ CMAKE_CONFIGURE_FLAGS: "-DWITH_IPV6=ON"
Review comment:
Why not just enable it for the base. IPv6 shouldn't exclude IPv4 support. Heck for that matter why not make it default to on?
##########
File path: cppcache/integration/test/BasicIPv6Test.cpp
##########
@@ -53,7 +55,11 @@ std::shared_ptr<Region> setupRegion(Cache& cache) {
* Example test using 2 servers and waiting for async tasks to synchronize using
* furtures.
*/
+#ifdef WITH_IPV6
+TEST(BasicIPv6Test, queryResultForRange) {
+#else
TEST(BasicIPv6Test, DISABLED_queryResultForRange) {
Review comment:
If I recall this test was disable because it relies on very specific address existing on the host or some specific address formatting. Something about it was not portable. We should just fix that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske closed pull request #765: GEODE-DIDN'TMAKEASTORYYET: Allow ipv6 test to be run
Posted by GitBox <gi...@apache.org>.
moleske closed pull request #765:
URL: https://github.com/apache/geode-native/pull/765
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on a change in pull request #765: GEODE-DIDN'TMAKEASTORYYET: Allow ipv6 test to be run
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #765:
URL: https://github.com/apache/geode-native/pull/765#discussion_r593477534
##########
File path: ci/base/base.yml
##########
@@ -96,4 +96,11 @@ builds:
image_family: build-ubuntu-20-04
source_image_family: ubuntu-2004-lts
+ - _: #@ template.replace(new_build("ubuntu-20.04-ipv6"))
+ image_family: build-ubuntu-20-04
+ source_image_family: ubuntu-2004-lts
+ #@yaml/map-key-override
+ params:
+ CMAKE_CONFIGURE_FLAGS: "-DWITH_IPV6=ON"
Review comment:
Fine with me!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on pull request #765: GEODE-DIDN'TMAKEASTORYYET: Allow ipv6 test to be run
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #765:
URL: https://github.com/apache/geode-native/pull/765#issuecomment-797813504
closing this to try some of the ideas discussed here. feel free to continue discussion if you stumble upon this later
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on a change in pull request #765: GEODE-DIDN'TMAKEASTORYYET: Allow ipv6 test to be run
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #765:
URL: https://github.com/apache/geode-native/pull/765#discussion_r593477610
##########
File path: cppcache/integration/test/BasicIPv6Test.cpp
##########
@@ -53,7 +55,11 @@ std::shared_ptr<Region> setupRegion(Cache& cache) {
* Example test using 2 servers and waiting for async tasks to synchronize using
* furtures.
*/
+#ifdef WITH_IPV6
+TEST(BasicIPv6Test, queryResultForRange) {
+#else
TEST(BasicIPv6Test, DISABLED_queryResultForRange) {
Review comment:
I think this comment got switched with the one about QueueConnectionRequestTest. QueueConnectionRequestTest is indeed not portable, but I didn't quite figure out how to deal with the different address formatting
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org