-
Notifications
You must be signed in to change notification settings - Fork 51
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
Adds Integration Testsuite for all supported runtimes, upgrades some deps #262
Conversation
< removed comment - integrated into description for brevity > |
This is ready for review now. I am interested in results on Mac as I tried only Linux and Windows. |
Hello, @rdebusscher , could you run the TS and review the MR? I would like to have it merged eventually 😃 Most importantly, could you run tests and see whether it works on Mac, please? |
src/it/java/org/eclipse/microprofile/starter/utils/ReadmeParser.java
Outdated
Show resolved
Hide resolved
src/it/java/org/eclipse/microprofile/starter/utils/ReadmeParser.java
Outdated
Show resolved
Hide resolved
src/it/java/org/eclipse/microprofile/starter/utils/WebpageTester.java
Outdated
Show resolved
Hide resolved
A general feedback: Thread.sleep was used in many places. Can it be replaced by Awaitability? |
@Emily-Jiang Do you mean https://github.com/awaitility/awaitility ? Could you elaborate more on what exactly is wrong with sleep in a loop until a condition OR timeout is reached? Given the usual dangers of Interruption and context - which I find kinda irrelevant in this particular code's context. I would like to have some reasoning for adding yet another library... |
See [src/it/README.md](src/it/README.md) for details. Signed-off-by: Michal Karm Babacek <karm@fedoraproject.org>
Signed-off-by: Michal Karm Babacek <karm@fedoraproject.org>
Resolved during call. The code is actually O.K. |
Signed-off-by: Michal Karm Babacek <karm@fedoraproject.org>
This is ready for another review. Changes addressed in the last commit. As usual, when it comes to bugs, please, do downvote the PR into oblivion as necessary :) |
Works after the update too... Linux
Results :
Windows
Those 4 failures on Windows are a known Payara bug: payara/Payara#4445 |
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.
LGTM!
See src/it/README.md for details on what the TS does and how.
Some components and dependencies changes to be looked at:
It works for all runtimes on Linux
It correctly hit the Helidon package problem, so it made me rebase. It also hit an error message from Liberty which I whitelisted. More on whitelisting is in src/it/README.md.
It is 24 integration tests for runtimes, 1 smoke check for the API before it all goes to runtimes and 5 unit tests.
The time it takes is pretty nice, but mind you I have all the artifacts cached in my local .m2 repo and I am running it on a fairly powerful workstation. When run fresh, it obviously downloads the Internet and takes much more time.
Ping @rdebusscher @cealsair @Emily-Jiang for thoughts.
Windows
Test suite fully supports Windows, although Payara does not pass because it doesn't start. See below.
Windows: Tests run: 30, Failures: 4, Errors: 0, Skipped: 0
Payara issue 4445
Payara fails not because of the test suite, but because of Payara.
@rdebusscher , could you follow up with your peers on (no examples selected, simplest case):
Please?
System