-
Notifications
You must be signed in to change notification settings - Fork 28
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 CLI testing utils #1235
Add CLI testing utils #1235
Conversation
@mocenas did you tested it locally, probably as part of test development for quarkus update command? |
Yes, this is tested locally. I already use it in development branch for quarkus update command. |
quarkus-test-cli/src/main/java/io/quarkus/test/util/QuarkusCLIUtils.java
Outdated
Show resolved
Hide resolved
quarkus-test-cli/src/main/java/io/quarkus/test/util/QuarkusCLIUtils.java
Outdated
Show resolved
Hide resolved
quarkus-test-cli/src/main/java/io/quarkus/test/util/QuarkusCLIUtils.java
Outdated
Show resolved
Hide resolved
quarkus-test-cli/src/main/java/io/quarkus/test/util/QuarkusCLIUtils.java
Outdated
Show resolved
Hide resolved
quarkus-test-cli/src/main/java/io/quarkus/test/util/QuarkusCLIUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I have requested only some small fixes.
To whoever will be merging this. Please don't forget to squash the commits. |
Done, also I've added few additional comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overal it's look good. Just have one change from my side and question about yaml.
import io.quarkus.test.bootstrap.QuarkusCliRestService; | ||
|
||
public abstract class QuarkusCLIUtils { | ||
public static final String RESOURCES_DIR = "src/main/resources"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work on Windows? I think as we using cgwin or similar enviroment it will but not 100% sure. But still I would prefer it similar to this https://github.com/quarkus-qe/quarkus-test-framework/blob/main/quarkus-test-core/src/main/java/io/quarkus/test/services/quarkus/QuarkusApplicationManagedResourceBuilder.java#L49.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I will change it.
writer.append(entry.getKey().toString()); | ||
writer.append(": "); | ||
writer.append(entry.getValue().toString()); | ||
writer.append("\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will yaml like this work. I imagine that this will work same as writePropertiesToApp
just the =
will be :
or the Properties
here will have yaml structure?
Also let say we have quarkus.log.level=DEBUG
What will be yaml output? Correct one should be:
quarkus:
log:
level: DEBUG
Note that if there is no indentation on new line the yaml will not work. If I would use it in future I would expect that I would be able to run that app at least by method name and it's javadoc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAML has benevolent syntax. The usually used one is the one you wrote here. But quarkus.log.level: DEBUG
is also possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for complex yaml, this doesn't work, so I'm fixing this one also.
} | ||
|
||
public static Properties readPropertiesYamlFile(QuarkusCliRestService app) throws IOException { | ||
return loadPropertiesFromFile(getPropertiesYamlFile(app)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with writing yaml. Will this work? I try it with yaml example above and this was output:
{log=, level=DEBUG, quarkus=}
To me it's look like 3 different properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately you're right :-(. Properties.load cannot really go through yaml. I will fix this to actually parse it.
Following jobs contain at least one flaky test: 'Linux - JVM build - Latest Version' |
Summary
Introduce QuarkusCLIUtils class. It contains supplementary methods for testing CLI apps. Like altering application's pom, properties file etc.
Most of this works with physical files and requires QuarkusCliRestService.getFileFromApplication to work. That is why I put it in quarkus-test-cli module.
IMHO it is not worth it to write examples/test for this, since most of the methods are actually helpers to check something.
Please check the relevant options
run tests
phrase in comment)Checklist: