GEODE-6183: Fix isAttachAPIFound and launcher tests #3650

kirklund · 2019-05-30T20:52:51Z

Co-authored-by: Michael Oleske moleske@pivotal.io
Co-authored-by: Mark Hanson mhanson@pivotal.io

Fixed StressNewTest job for all launcher tests. These were failing in StressNewTest. I also moved all files to TemporaryFolder.

Additional reviewers: @demery-pivotal @mhansonp @moleske @aaronlindsey

Note: due to the empty commit to trigger precheckin, I will do a rebase and force push after reviews but before the final merge to develop.

upthewaterspout

I haven't reviewed all of the formatting cleanup, but the actual bug fix looks good. Many thanks for separating out the code cleanup from the actual fix!

This is beyond the scope of this PR, but we really shouldn't be using this Attach API at all. There are way better and more standard ways to interact with a process we've launched.

kirklund · 2019-06-05T21:59:57Z

@upthewaterspout I think the only feature we're still using in Attach API that isn't available elsewhere is the check isProcessAlive(int pid) for any pid (not a java.lang.Process forked from the current JVM). I'm all for removing it. EDIT: actually NativeCalls (JNA) does have support for it.

moleske

Yay code cleanup (and the fix too)! Is there an existing Geode JIRA story for @upthewaterspout 's point about not using this?

metatype · 2019-06-05T23:36:35Z

Maybe: ProcessHandle.of(pid).ifExists(ProcessHandle::isAlive)

kirklund · 2019-06-06T17:08:17Z

@moleske @upthewaterspout I filed GEODE-6841: Remove dependency on Attach API

kirklund · 2019-06-06T17:10:27Z

Maybe: ProcessHandle.of(pid).ifExists(ProcessHandle::isAlive)

@metatype What API is ProcessHandle part of? My IntelliJ project cannot find that class. Also, please update https://issues.apache.org/jira/browse/GEODE-6841 with any info on alternative APIs. Please provide a review as well (we need 3) -- thanks! EDIT: Looks like ProcessHandle isn't added until JDK 9 source level.

geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorIntegrationTest.java

...rc/integrationTest/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java

kohlmu-pivotal · 2019-06-06T18:02:28Z

I might be archaic in my thinking or understanding, but I thought there was some guidance that was once provided that inherited Test classes were a smell. If so, I do see quite a few of the test classes use inheritance, which makes this review harder, as I would now have to go back to source code to understand what a specific method would be providing.
In addition, I also see some classes are names "Integration". I thought there was an integration category that was used. Maybe the category was missed or maybe renaming of the Test class to further its classification would be more helpful.
Overall, these changes look good, with only cosmetic and hopefully minor changes

bschuchardt

The locator integration test changes look fine. There's no diff for the distributed system integration test - can't tell what changed there. As for the original commit/fix it looks okay to me but I'm not familiar with the Attach API

kirklund · 2019-06-06T22:07:19Z

@kohlmu-pivotal If I have a class called LocatorLauncher, then I create a unit test in src/test called LocatorLauncherTest and an integration test in src/integrationTest called LocatorLauncherIntegrationTest. I use those names simply because I want to. That's what I documented on the Geode wiki in the past and have been using for years.

kirklund · 2019-06-06T22:08:38Z

@bschuchardt The diff for DistributedSystemIntegrationTest is 4th commit down in the commits tab. Here's a URL to it: e3d8cfb

kirklund · 2019-06-06T22:11:27Z

@kohlmu-pivotal Re: inherited test classes: These are not new tests that I just recently decided to create with multiple-class inheritance. I simply cleaned them up. I'm not going to rewrite them from scratch, but you certainly can if you really want to.

kohlmu-pivotal · 2019-06-06T23:14:09Z

@kirklund I thought that we had an @Category("IntegrationTest") that was there.. but I have not created a new test in a while
I also understand that you did just refactor the test. I was merely calling out that I believed that test inheritance was a smell that was agreed upon.
As I also stated, overall the changes look cosmetic and acceptable.

rhoughton-pivot · 2019-06-10T18:09:24Z

geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorIntegrationTest.java

-      assertEquals(GemFireCacheImpl.getInstance().getMyId(),
-          alreadyManaging.get(0).getDistributedMember());
-    } finally {
-      System.clearProperty(DistributionConfig.GEMFIRE_PREFIX + "enabledManagement");


Is it correct to leave this System variable set after the test completes?

@rhoughton-pivot Adding RestoreSystemProperties JUnit Rule restores all System Properties after each test.

aaronlindsey · 2019-06-10T18:39:24Z

geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorIntegrationTest.java

-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;


What's the motivation behind using AssertJ instead of JUnit? How do we decide whether to use one vs the other?

AssertJ is a much richer assertion API that introduced to Geode a few years ago. I highly recommend using it over JUnit assertions!

@aaronlindsey: I think the general preference and consensus is to use AssertJ instead of JUnit, it's been discussed some time ago in the Geode dev list: https://markmail.org/message/al3st2gko4hddwo2.

kirklund · 2019-06-13T17:21:08Z

Review is closed. I'm going to do one final rebase on develop, force-push to remove the empty commit (for precheckin trigger). and then merge to develop.

VirtualMachine class can be loaded even if AttachProvider service providers are not found. This change forces loading of providers for isAttachAPIFound. Co-authored-by: Michael Oleske <moleske@pivotal.io>

…tegrationTest The tests that were using InetAddress.getLostHost for assertions may fail depending on /etc/hosts configuration. This change should prevent these failures. Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: Mark Hanson <mhanson@pivotal.io>

* Fixup IDE warnings * Reformat test code * Use SocketCreator.getLocalHost

* Fixup IDE warnings * Reformat test code

* Fixup IDE warnings * Reformat test code * Update to use AssertJ

kirklund force-pushed the GEODE-6183-WindowsAttachProvider branch 2 times, most recently from 22aa84e to 675c816 Compare June 3, 2019 22:31

kirklund marked this pull request as ready for review June 4, 2019 16:09

kirklund requested review from moleske and jdeppe-pivotal June 4, 2019 16:09

kirklund force-pushed the GEODE-6183-WindowsAttachProvider branch from 675c816 to 3d3e547 Compare June 4, 2019 18:36

kirklund changed the title ~~DRAFT: GEODE-6183: Fix isAttachAPIFound and launcher tests~~ GEODE-6183: Fix isAttachAPIFound and launcher tests Jun 4, 2019

kirklund requested review from bschuchardt, kohlmu-pivotal, pivotal-amurmann, metatype, jake-at-work, upthewaterspout and onichols-pivotal June 5, 2019 20:57

upthewaterspout approved these changes Jun 5, 2019

View reviewed changes

moleske approved these changes Jun 5, 2019

View reviewed changes

dschneider-pivotal approved these changes Jun 6, 2019

View reviewed changes

kohlmu-pivotal reviewed Jun 6, 2019

View reviewed changes

geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorIntegrationTest.java Show resolved Hide resolved

kohlmu-pivotal reviewed Jun 6, 2019

View reviewed changes

...rc/integrationTest/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java Show resolved Hide resolved

bschuchardt reviewed Jun 6, 2019

View reviewed changes

rhoughton-pivot reviewed Jun 10, 2019

View reviewed changes

aaronlindsey reviewed Jun 10, 2019

View reviewed changes

kirklund and others added 5 commits June 13, 2019 10:21

GEODE-6183: Make isAttachAPIFound load providers

8fc077e

VirtualMachine class can be loaded even if AttachProvider service providers are not found. This change forces loading of providers for isAttachAPIFound. Co-authored-by: Michael Oleske <moleske@pivotal.io>

GEODE-6183: Cleanup Launcher integration tests

5e5019d

* Fixup IDE warnings * Reformat test code * Use SocketCreator.getLocalHost

GEODE-6183: Cleanup and rename DistributedSystemIntegrationTest

d0abb5c

* Fixup IDE warnings * Reformat test code

GEODE-6183: Cleanup and rename LocatorIntegrationTest

c566cd7

* Fixup IDE warnings * Reformat test code * Update to use AssertJ

kirklund force-pushed the GEODE-6183-WindowsAttachProvider branch from 40934ab to c566cd7 Compare June 13, 2019 17:21

kirklund merged commit 87c83ee into apache:develop Jun 13, 2019

kirklund deleted the GEODE-6183-WindowsAttachProvider branch June 13, 2019 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-6183: Fix isAttachAPIFound and launcher tests #3650

GEODE-6183: Fix isAttachAPIFound and launcher tests #3650

kirklund commented May 30, 2019 •

edited

upthewaterspout left a comment

kirklund commented Jun 5, 2019 •

edited

moleske left a comment

metatype commented Jun 5, 2019

kirklund commented Jun 6, 2019

kirklund commented Jun 6, 2019 •

edited

kohlmu-pivotal commented Jun 6, 2019

bschuchardt left a comment

kirklund commented Jun 6, 2019

kirklund commented Jun 6, 2019

kirklund commented Jun 6, 2019

kohlmu-pivotal commented Jun 6, 2019

rhoughton-pivot Jun 10, 2019

kirklund Jun 10, 2019 •

edited

aaronlindsey Jun 10, 2019

kirklund Jun 10, 2019

jujoramos Jun 10, 2019 •

edited

kirklund commented Jun 13, 2019

GEODE-6183: Fix isAttachAPIFound and launcher tests #3650

GEODE-6183: Fix isAttachAPIFound and launcher tests #3650

Conversation

kirklund commented May 30, 2019 • edited

upthewaterspout left a comment

Choose a reason for hiding this comment

kirklund commented Jun 5, 2019 • edited

moleske left a comment

Choose a reason for hiding this comment

metatype commented Jun 5, 2019

kirklund commented Jun 6, 2019

kirklund commented Jun 6, 2019 • edited

kohlmu-pivotal commented Jun 6, 2019

bschuchardt left a comment

Choose a reason for hiding this comment

kirklund commented Jun 6, 2019

kirklund commented Jun 6, 2019

kirklund commented Jun 6, 2019

kohlmu-pivotal commented Jun 6, 2019

rhoughton-pivot Jun 10, 2019

Choose a reason for hiding this comment

kirklund Jun 10, 2019 • edited

Choose a reason for hiding this comment

aaronlindsey Jun 10, 2019

Choose a reason for hiding this comment

kirklund Jun 10, 2019

Choose a reason for hiding this comment

jujoramos Jun 10, 2019 • edited

Choose a reason for hiding this comment

kirklund commented Jun 13, 2019

kirklund commented May 30, 2019 •

edited

kirklund commented Jun 5, 2019 •

edited

kirklund commented Jun 6, 2019 •

edited

kirklund Jun 10, 2019 •

edited

jujoramos Jun 10, 2019 •

edited