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

MP Health test development #87

Merged
merged 1 commit into from
Dec 13, 2019
Merged

Conversation

istraka
Copy link
Member

@istraka istraka commented Dec 11, 2019

refactoring
negative scenario for MIME types
multiple deployment / deployment with subdeployments scenario
new health version test
JavaDoc

https://eap-qe-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/eap-7.x-microprofile-simple-face/11/

Please make sure your PR meets the following requirements:

  • Pull Request contains description for the changes
  • Pull Request does not include fixes for multiple issues / topics
  • Code is formatted, imports ordered, code compiles and tests are passing
  • Link to passing job is provided
  • Code is self-descriptive and/or documented
  • Description of the tests scenarios is included (see Update PR template to include TPG stuff #46)

@istraka
Copy link
Member Author

istraka commented Dec 11, 2019

Jenkins is down for now. Here is a distribution (available only from RedHat network) http://download.eng.brq.redhat.com/scratch/istraka/wildfly-mp-ft-health.zip.

The distribution is a merge of wildfly/wildfly#12800 and wildfly/wildfly#12720

Copy link
Member

@fabiobrz fabiobrz left a comment

Choose a reason for hiding this comment

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

Hi @istraka, thanks for your work.
I've left some minor comments and a broader consideration regarding multi deployment scenarios (see MultiAppHealthTest and subclasses).

microprofile-health/pom.xml Outdated Show resolved Hide resolved

@ContainerResource
ManagementClient managementClient;

@Deployment(testable = false)
public static Archive<?> deployment() {
WebArchive war = ShrinkWrap.create(WebArchive.class, "MicroProfileHealthNullTest.war")
WebArchive war = ShrinkWrap.create(WebArchive.class, HealthTest.class.getSimpleName() + ".war")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you meant HealthTest.class.getSimpleName() -> HealthNullTest.class.getSimpleName()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing...

.contentType(ContentType.JSON)
.header("Content-Type", containsString("application/json"))
.body("status", is("UP"),
"checks", hasSize(3), // BothHealthCheck contains both annotations: @Liveness and @Readiness
Copy link
Member

@fabiobrz fabiobrz Dec 11, 2019

Choose a reason for hiding this comment

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

Looks like BothHealthCheck is not deployed by any of the two classes extending this one (maybe you meant SimplifiedHealthTest which as well has both annotations.
Anyway, generally speaking, I am concerned about putting this kind of logic into this abstract superclass, while the control over what is deployed is in concrete subclass(es). In fact a comment had to be added to track this.
WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I see your point. The abstract class tests expect sub-classes to deploy something specific / limited. What about having a sort of TestDelegation class instead of this abstract class?

Copy link
Member

Choose a reason for hiding this comment

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

The abstract class tests expect sub-classes to deploy something specific / limited.

Correct. The key point of my consideration is to not scatter implementation logic over hierarchy.
Any "pattern" that would solve this is good IMO. Delegation to run some supplied code would work, I guess, but then what's the point for that "empty" superclass?
I - for instance - would start from removing the "details-aware" code from the superclass to subclasses - i.e. I'd move the checks down, but that would just be my immediate coding attempt.
Then the design could be adjusted further, somebody else could choose composition.
I'd say there are several solutions and you know better the detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I would get rid of the hierarchy....Fixing...

pom.xml Outdated
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-io</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

What is this dependency used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tests brought by next iteration will use it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Tests brought by next iteration will use it.

please don't add it here, viz "Pull Request does not include fixes for multiple issues / topics" checkbox

pom.xml Outdated
@@ -132,6 +143,9 @@
<artifactId>maven-surefire-plugin</artifactId>
<version>2.22.2</version>
<configuration>
<environmentVariables>
<JBOSS_HOME>${jboss.home}</JBOSS_HOME>
</environmentVariables>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this environment variable needed to be passed to surefire now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of Creaper. Not used now, but in following PR it will be needed.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a short explanation for this? I think it is needed for it to be shared here. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can not execute CLI command using Creaper

ERROR: Setup failed during setup. Offending class 'org.jboss.eap.qe.microprofile.health.integration.FailSafeCDICustomConfigSourceHealthTest$SetupTask@381d7219'
org.wildfly.extras.creaper.core.CommandFailedException: org.wildfly.extras.creaper.core.online.CliException: org.jboss.as.cli.CommandLineException: java.util.concurrent.ExecutionException: org.jboss.as.cli.CommandLineException: JBOSS_HOME environment variable is not set.
	at org.wildfly.extras.creaper.core.online.AutomaticErrorHandlingForCommands.commandFailedWithCause(AutomaticErrorHandlingForCommands.java:142)
	at org.wildfly.extras.creaper.core.online.AutomaticErrorHandlingForCommands.executeCli(AutomaticErrorHandlingForCommands.java:109)
	at org.wildfly.extras.creaper.commands.modules.AddModule.apply(AddModule.java:91)
	at org.wildfly.extras.creaper.core.online.OnlineManagementClientImpl.apply(OnlineManagementClientImpl.java:136)
	at org.wildfly.extras.creaper.core.online.OnlineManagementClientImpl.apply(OnlineManagementClientImpl.java:125)
	at org.wildfly.extras.creaper.core.online.LazyOnlineManagementClient.apply(LazyOnlineManagementClient.java:50)
	at org.jboss.eap.qe.microprofile.health.integration.ModuleUtil.setupModule(ModuleUtil.java:21)
	at org.jboss.eap.qe.microprofile.health.integration.FailSafeCDICustomConfigSourceHealthTest$SetupTask.setup(FailSafeCDICustomConfigSourceHealthTest.java:71)
	at org.jboss.as.arquillian.container.ServerSetupObserver$ServerSetupTaskHolder.setup(ServerSetupObserver.java:185)
	at org.jboss.as.arquillian.container.ServerSetupObserver.handleBeforeDeployment(ServerSetupObserver.java:108)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
	at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
	at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
	at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$3.call(ContainerDeployController.java:147)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$3.call(ContainerDeployController.java:118)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.executeOperation(ContainerDeployController.java:239)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.deploy(ContainerDeployController.java:118)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
	at org.jboss.arquillian.container.impl.client.ContainerDeploymentContextHandler.createDeploymentContext(ContainerDeploymentContextHandler.java:71)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
	at org.jboss.arquillian.container.impl.client.ContainerDeploymentContextHandler.createContainerContext(ContainerDeploymentContextHandler.java:54)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
	at org.jboss.arquillian.container.impl.client.container.DeploymentExceptionHandler.verifyExpectedExceptionDuringDeploy(DeploymentExceptionHandler.java:47)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
	at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
	at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
	at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$1.perform(ContainerDeployController.java:92)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$1.perform(ContainerDeployController.java:77)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.forEachDeployment(ContainerDeployController.java:232)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.forEachManagedDeployment(ContainerDeployController.java:212)
	at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.deployManaged(ContainerDeployController.java:77)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
	at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
	at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
	at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
	at org.jboss.arquillian.container.test.impl.client.ContainerEventController.execute(ContainerEventController.java:96)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
	at org.jboss.arquillian.test.impl.TestContextHandler.createClassContext(TestContextHandler.java:83)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
	at org.jboss.arquillian.test.impl.TestContextHandler.createSuiteContext(TestContextHandler.java:69)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
	at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
	at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
	at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
	at org.jboss.arquillian.test.impl.EventTestRunnerAdaptor.beforeClass(EventTestRunnerAdaptor.java:89)
	at org.jboss.arquillian.junit.Arquillian$2.evaluate(Arquillian.java:163)
	at org.jboss.arquillian.junit.Arquillian.multiExecute(Arquillian.java:350)
	at org.jboss.arquillian.junit.Arquillian.access$200(Arquillian.java:54)
	at org.jboss.arquillian.junit.Arquillian$3.evaluate(Arquillian.java:177)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.jboss.arquillian.junit.Arquillian.run(Arquillian.java:115)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:365)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:273)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
Caused by: org.wildfly.extras.creaper.core.online.CliException: org.jboss.as.cli.CommandLineException: java.util.concurrent.ExecutionException: org.jboss.as.cli.CommandLineException: JBOSS_HOME environment variable is not set.
	at org.wildfly.extras.creaper.core.online.OnlineManagementClientImpl.executeCli(OnlineManagementClientImpl.java:210)
	at org.wildfly.extras.creaper.core.online.AutomaticErrorHandlingForCommands.executeCli(AutomaticErrorHandlingForCommands.java:107)
	... 107 more
Caused by: org.jboss.as.cli.CommandLineException: java.util.concurrent.ExecutionException: org.jboss.as.cli.CommandLineException: JBOSS_HOME environment variable is not set.
	at org.jboss.as.cli.impl.CommandContextImpl.execute(CommandContextImpl.java:878)
	at org.jboss.as.cli.impl.CommandContextImpl.handleLegacyCommand(CommandContextImpl.java:1840)
	at org.jboss.as.cli.impl.CommandContextImpl.handle(CommandContextImpl.java:815)
	at org.jboss.as.cli.impl.WorkaroundForWFCORE526_CommandContextImpl.handle(WorkaroundForWFCORE526_CommandContextImpl.java:14)
	at org.wildfly.extras.creaper.core.online.OnlineManagementClientImpl.executeCli(OnlineManagementClientImpl.java:203)
	... 108 more
Caused by: java.util.concurrent.ExecutionException: org.jboss.as.cli.CommandLineException: JBOSS_HOME environment variable is not set.
	at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191)
	at org.jboss.as.cli.impl.CommandExecutor.execute(CommandExecutor.java:713)
	at org.jboss.as.cli.impl.CommandExecutor.execute(CommandExecutor.java:694)
	at org.jboss.as.cli.impl.CommandContextImpl.lambda$handleLegacyCommand$4(CommandContextImpl.java:1841)
	at org.jboss.as.cli.impl.CommandContextImpl.execute(CommandContextImpl.java:865)
	... 112 more
Caused by: org.jboss.as.cli.CommandLineException: JBOSS_HOME environment variable is not set.
	at org.jboss.as.cli.handlers.module.ASModuleHandler.getModulesDir(ASModuleHandler.java:570)
	at org.jboss.as.cli.handlers.module.ASModuleHandler.addModule(ASModuleHandler.java:395)
	at org.jboss.as.cli.handlers.module.ASModuleHandler.doHandle(ASModuleHandler.java:346)
	at org.jboss.as.cli.handlers.CommandHandlerWithHelp.handle(CommandHandlerWithHelp.java:89)
	at org.jboss.as.cli.impl.CommandExecutor$2.lambda$build$0(CommandExecutor.java:685)
	at org.jboss.as.cli.impl.CommandExecutor.lambda$execute$0(CommandExecutor.java:708)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:832)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, not really "short" but definitely valuable :)

pom.xml Outdated
@@ -122,6 +123,16 @@
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.wildfly.extras.creaper</groupId>
<artifactId>creaper-commands</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment about creaper-commands

Copy link
Member Author

Choose a reason for hiding this comment

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

Next iteration.

Copy link
Member

Choose a reason for hiding this comment

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

OK thanks, same as above.

.contentType(ContentType.JSON)
.header("Content-Type", containsString("application/json"))
.body("status", is("UP"),
"checks", hasSize(2), // BothHealthCheck contains both annotations: @Liveness and @Readiness
Copy link
Member

Choose a reason for hiding this comment

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

BothHealthCheck -> SimplifiedHealthTest here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing...

}

/**
* @tpTestDetails Customer scenario where health check contains customer data.
Copy link
Member

Choose a reason for hiding this comment

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

health check -> liveness check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing...

@@ -74,6 +83,11 @@ public void testLivenessEndpoint() {
"checks.data[0..1].key", hasItems("value"));
}

/**
* @tpTestDetails Customer scenario where health check contains customer data.
Copy link
Member

Choose a reason for hiding this comment

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

health check -> readiness check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing...

@istraka
Copy link
Member Author

istraka commented Dec 12, 2019

@fabiobrz I have addressed issues. At the end I haven't implemented any sort of test delegation class since it is too complicated for the purpose. I just get rid of the hierarchy. Let me know if you are fine with changes so I would squash commits.

Copy link
Member

@fabiobrz fabiobrz left a comment

Choose a reason for hiding this comment

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

Hi @istraka, thanks for addressing my comments. See my request for documenting right here in the PR what's the future usage for environment variable passed to surefire. Then there are just some minor fixes. Maybe, if Jenkin's now up, we could have a run linked, here.

microprofile-health/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated
@@ -122,6 +123,16 @@
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.wildfly.extras.creaper</groupId>
<artifactId>creaper-commands</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

OK thanks, same as above.

pom.xml Outdated
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-io</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks.

pom.xml Outdated
@@ -132,6 +143,9 @@
<artifactId>maven-surefire-plugin</artifactId>
<version>2.22.2</version>
<configuration>
<environmentVariables>
<JBOSS_HOME>${jboss.home}</JBOSS_HOME>
</environmentVariables>
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a short explanation for this? I think it is needed for it to be shared here. Thanks.

.contentType(ContentType.JSON)
.header("Content-Type", containsString("application/json"))
.body("status", is("UP"),
"checks", hasSize(3), // BothHealthCheck contains both annotations: @Liveness and @Readiness
Copy link
Member

Choose a reason for hiding this comment

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

Here again I think it's BothHealthCheck -> SimplifiedHealthCheck or just remove the comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing...

.contentType(ContentType.JSON)
.header("Content-Type", containsString("application/json"))
.body("status", is("UP"),
"checks", hasSize(3), // BothHealthCheck contains both annotations: @Liveness and @Readiness
Copy link
Member

Choose a reason for hiding this comment

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

BothHealthCheck -> SimplifiedHealthCheck or just remove the comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing...

@istraka
Copy link
Member Author

istraka commented Dec 12, 2019

Comments were removed.

Copy link
Member

@fabiobrz fabiobrz left a comment

Choose a reason for hiding this comment

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

Hi @istraka, LGTM, approving.

pom.xml Outdated
@@ -132,6 +143,9 @@
<artifactId>maven-surefire-plugin</artifactId>
<version>2.22.2</version>
<configuration>
<environmentVariables>
<JBOSS_HOME>${jboss.home}</JBOSS_HOME>
</environmentVariables>
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, not really "short" but definitely valuable :)

@rsvoboda
Copy link
Member

@istraka rebase please

@mnovak
Copy link
Member

mnovak commented Dec 12, 2019

@istraka I would like to see improved JavaDoc with test description so it's clear what the test is checking. Most of the javadoc is too general.

@istraka istraka force-pushed the mp-health branch 2 times, most recently from 64b082e to f5d61ab Compare December 12, 2019 17:30
@istraka
Copy link
Member Author

istraka commented Dec 12, 2019

@mnovak1 I have addressed issues. Please review.

@istraka istraka force-pushed the mp-health branch 2 times, most recently from 40c3ca4 to 713b20f Compare December 13, 2019 10:00
Copy link
Member

@mnovak mnovak left a comment

Choose a reason for hiding this comment

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

Almost LGTM.

	refactoring
	negative scenario for MIME types
	multiple deployment / deployment with subdeployments scenario
	new health version test
	JavaDoc
@mnovak mnovak merged commit 86fac04 into jboss-eap-qe:master Dec 13, 2019
@istraka
Copy link
Member Author

istraka commented Jan 2, 2020

Fixes #14

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.

4 participants