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

Introduce @QuarkusIntegrationTest #15009

Closed
wants to merge 1 commit into from
Closed

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Feb 11, 2021

The basic idea of this annotation is to test the final artifact produced by the Quarkus build, no matter what that is (a jar, a native binary or a container image).

TODO:

  • Polish code
  • Add tests
  • Add documentation

Resolves: #13818

@geoand
Copy link
Contributor Author

geoand commented Feb 11, 2021

This will conflict with #14960, so I'll wait until that one is merged

@geoand
Copy link
Contributor Author

geoand commented Feb 12, 2021

I have now added tests and cleaned things up a little.

@stuartwdouglas mind taking a look and letting me know what you think?

@ebramirez
Copy link
Contributor

Hi @geoand, could this be extended to include testing quarkus-picocli applications?

@geoand
Copy link
Contributor Author

geoand commented Feb 12, 2021

I am not sure, I haven't really worked with the picocli extension.

What do you expect testing of a picocli application would look like?

@ebramirez
Copy link
Contributor

I imagine that given a Picocli command like this:

@Command
public class MyCommand {
    @Parameters( paramLabel = "PARAM", description = "App parameter", arity = "1" )
    Path param;

    @Option( names = { "--option", "-o" }, required = true )
    String option;

	/* Setters and Getters */
}

We could test it like this:

@QuarkusIntegrationTest
class MyCommandTest {

    @Inject
    MyCommand myCommand;

    @Test
    void testValidOptions() {
        myCommand.setPath( "/my/path/param.txt" );
		myCommand.setOption( "my option" );

        final int exitStatus = myCommand.execute();

        assertThat( exitStatus ).isEqualTo( 0 );
    }
}

The execute() method from the injected command would pick up the annotated Options and Parameters and pass them to the appropriate launcher.

@geoand
Copy link
Contributor Author

geoand commented Feb 12, 2021

That can't work with this type of testing as the test launches a separate process and thus cannot interact with the application in any way other than standard ways to interact with a separate process - which is just a fancy way of say that @Inject can't work in these tests

@ebramirez
Copy link
Contributor

Sorry for not explaining myself properly: the injected Command would not be the real command, just a proxy to collect the Options and Properties and pass them to the appropriate launcher (Jar, Native, Docker...).

Maybe something similar, but more automated, to using @InjectMock and @RestClient.

Does this make sense.

@geoand
Copy link
Contributor Author

geoand commented Feb 13, 2021

Thanks for the explanation.

That is probably possible and I'll consider it in the future.

try {
while ((i = inputStream.read(b)) > 0) {
String str = new String(b, 0, i, StandardCharsets.UTF_8);
System.out.print(str);
Copy link
Member

Choose a reason for hiding this comment

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

Its probably a massive corner case that we don't care about, but this won't display properly if you have a muti byte character that falls on the 100 byte mark.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a corner case indeed 🙂.
But I'll keep it in mind and deal with it if it ever becomes a problem.

}
ArtifactLauncher launcher;
switch (artifactType) {
case "native": {
Copy link
Member

Choose a reason for hiding this comment

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

This is ok for now, but I think long term we want the extension that contributes the artifact to also provide the launcher somehow to make this extensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely. I was thinking of having a Service loader.
Once that need arises, I'll change the code ASAP

@geoand
Copy link
Contributor Author

geoand commented Feb 15, 2021

I'm gonna keep this PR in draft until we get DevDB support in as they will conflict and the DevDB support is more high value

@geoand geoand force-pushed the #13818 branch 6 times, most recently from 8f9f041 to ba08625 Compare February 16, 2021 18:17
@geoand
Copy link
Contributor Author

geoand commented Feb 16, 2021

At last the Windows tests on my fork as passing

@pipinet
Copy link

pipinet commented Feb 19, 2021

#15198

@geoand
Copy link
Contributor Author

geoand commented Feb 22, 2021

Fixed conflict

@geoand geoand closed this Feb 22, 2021
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Feb 22, 2021
@geoand
Copy link
Contributor Author

geoand commented Feb 22, 2021

GH seems to have messed up this PR.

I'll try to reopen later on or else just open a new one

@geoand
Copy link
Contributor Author

geoand commented Feb 22, 2021

Superseded by #15227

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NativeImageTest with container-image extensions
4 participants