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

add GradleRunIntegrationTest (incl. QuarkusJavaLauncher) #1986

Conversation

vorburger
Copy link
Contributor

@vorburger vorburger commented Apr 10, 2019

@vorburger vorburger force-pushed the issue-1623_actually-add-gradle-it-run_ch.vorburger.exec branch from 00d389b to 73a9117 Compare April 10, 2019 17:12
@vorburger
Copy link
Contributor Author

vorburger commented Apr 11, 2019

@stalep FYI this may actually help to understand the cause of the build failure in #1985 better (because {my} exec util logs much more than j.l.Process), so unless there are strong objections to the inclusion of that 3rd party lib, I think this provides real value.

I'll therefore ASAP rebase this based on recent merge (of #1993 and #1929) - let's see if we get more details about the failure (only) on Azure with this...

@vorburger vorburger force-pushed the issue-1623_actually-add-gradle-it-run_ch.vorburger.exec branch from 73a9117 to cb3092f Compare April 11, 2019 12:32
@stalep
Copy link
Member

stalep commented Apr 11, 2019

I'm just not sure the reason to add another dependency for this (which contain other dependencies :) other than just for logging. What kind of output is omitted when using standard Process vs your impl?

@vorburger
Copy link
Contributor Author

What kind of output is omitted when using standard Process vs your impl?

There simply is no output at all... 😈 it would, of course, be possible to re-implement the equivalent, but (to me) this seems to belong in some library - which is, years ago, I built https://github.com/vorburger/ch.vorburger.exec, and am happy to also offer it to Quarkus... 😸 See also https://github.com/vorburger/ch.vorburger.exec#advantages. (BTW: I'm happy to "donate" it under some JBoss.org repo, if there is interest, or accept other committers to the project.)

help to understand the cause of the build failure in #1985 better
see if we get more details about the failure (only) on Azure with this...

And indeed it does!! FTR, locally it's like this:

[INFO] Running io.quarkus.gradle.it.GradleRunIntegrationTest
[main] INFO ch.vorburger.exec.ManagedProcess - Starting Program [java, -jar, ../gradle-it/build/gradle-it-runner.jar] (in working directory /home/vorburger/Quarkus/git/quarkus/devtools/gradle-it-run/.)
[Exec Stream Pumper] INFO ch.vorburger.exec.ManagedProcess - java: 2019-04-11 12:32:17,896 INFO  [io.quarkus] (main) Quarkus 999-SNAPSHOT started in 3.055s. Listening on: http://[::]:8080
[Exec Stream Pumper] INFO ch.vorburger.exec.ManagedProcess - java: 2019-04-11 12:32:18,007 INFO  [io.quarkus] (main) Installed features: [cdi, resteasy]
[main] INFO ch.vorburger.exec.ManagedProcess - Successfully destroyed Program [java, -jar, ../gradle-it/build/gradle-it-runner.jar] (in working directory /home/vorburger/Quarkus/git/quarkus/devtools/gradle-it-run/.)
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.591 s - in io.quarkus.gradle.it.GradleRunIntegrationTest

on the Azure build it's like this:

[INFO] Running io.quarkus.gradle.it.GradleRunIntegrationTest
[main] INFO ch.vorburger.exec.ManagedProcess - Starting Program [java, -jar, ../gradle-it/build/gradle-it-runner.jar] (in working directory /home/vsts/work/1/s/devtools/gradle-it-run/.)
[main] INFO ch.vorburger.exec.ManagedProcess - Successfully destroyed Program [java, -jar, ../gradle-it/build/gradle-it-runner.jar] (in working directory /home/vsts/work/1/s/devtools/gradle-it-run/.)
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.494 s <<< FAILURE! - in io.quarkus.gradle.it.GradleRunIntegrationTest
[ERROR] testHelloEndpoint  Time elapsed: 1.479 s  <<< ERROR!
java.net.ConnectException: Connection refused (Connection refused)

I now see the problem - and will ASAP amend this PR with what I think will fix it...

@vorburger
Copy link
Contributor Author

The build failure (above) on the Build_JDK11_Linux Job is due to #2012 ... @stalep BTW this underlines how important it is to run tests like this and be able to see the output from the externally launched process. If this new GradleRunIntegrationTest, and more like it in the future (e.g. #2005) were to use only simple j.l.ProcessBuilder directly, it would be almost impossible to make sense of a test failure like the #2012 problem (trust me, I've been there, in MariaDB4j).

@vorburger vorburger force-pushed the issue-1623_actually-add-gradle-it-run_ch.vorburger.exec branch from 8836a9e to d2ce2ea Compare April 12, 2019 12:42
@stalep
Copy link
Member

stalep commented Apr 12, 2019

hm, the windows build failed again.

@vorburger
Copy link
Contributor Author

hm, the windows build failed again

not just the Windows build but the Java 11 one as well 😢 see #2036, for some reason the built JAR is bad, it has INFO [io.quarkus] (main) Installed features: [] instead of [cdi, resteasy] - but I've no clue what could cause this, on the Azure build; works fine locally!

@vorburger vorburger changed the title use exec util instead of raw ProcessBuilder in GradleRunIntegrationTest add GradleRunIntegrationTest (incl. QuarkusJavaLauncher) Apr 12, 2019
@geoand
Copy link
Contributor

geoand commented Apr 12, 2019

I also tried this PR locally with both Java 8 and Java 11 and it worked.

@vorburger
Copy link
Contributor Author

@stalep @cescoffier @aloubyansky @gsmet @stuartwdouglas I'd love for this work to be able to be merged, but am quite clueless as to why the Gradle build produces a JAR with empty Installed Features - and thus fails this new IT, but only on the Azure build; it works locally, for me, @geoand and (I think) @stalep.

I'm happy to rebase this once - if someone can then help to investigate the build failure further?

<maven.deploy.skip>true</maven.deploy.skip>
</properties>

<dependencies>
Copy link
Member

Choose a reason for hiding this comment

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

This does not have a dependency on quarkus-gradle-plugin-integration-test so its possible this can run before the jar it depends on is built.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuartwdouglas that's a good point, I've added a dependency now - am curious if this helps to get this PR to finally pass on the Azure build server as well (and not just locally, as it always has)

@cescoffier
Copy link
Member

I'm a bit worried about this PR pending for so long. @stalep when you have a few minutes can you tell me the current state?

@cescoffier cescoffier added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Jun 7, 2019
@vorburger
Copy link
Contributor Author

#2214 removed the devtools/gradle-it which this is based on, so that has to come back for this...

@vorburger vorburger force-pushed the issue-1623_actually-add-gradle-it-run_ch.vorburger.exec branch from d2ce2ea to bac56cc Compare June 10, 2019 21:32
@vorburger
Copy link
Contributor Author

#2214 removed the devtools/gradle-it which this is based on, so that has to come back for this...

taken care of in #2783

cescoffier added the needs-rebase label 4 days ago

Rebase done (based on / assuming #2783, which is included here)

tell me the current state?

It used to build fine locally and fail (only) on the Azure build; let's see if it's better now...

@emmanuelbernard
Copy link
Member

Ping @stalep

@dmlloyd
Copy link
Member

dmlloyd commented Jul 10, 2019

Ping x2 @stalep? I'll fire off a more recent CI but it would be good to get a review on this.

@dmlloyd
Copy link
Member

dmlloyd commented Jul 10, 2019

Never mind: I don't have permissions to re-queue a successful build. Also the build is so old that it has disappeared off Azure. So I guess... maybe force-push it one more time?

@geoand
Copy link
Contributor

geoand commented Jul 10, 2019

Is this superceded by #2783 ?

@emmanuelbernard
Copy link
Member

This one is likely very outdated. I'm going to close it. Open it again if I'm incorrect.

@gsmet gsmet added the triage/out-of-date This issue/PR is no longer valid or relevant label Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/needs-rebase This PR needs to be rebased first because it has merge conflicts triage/out-of-date This issue/PR is no longer valid or relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gradle Plugin Integration Test should, ideally, actually run the application
8 participants