-
-
Notifications
You must be signed in to change notification settings - Fork 314
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
Completed prep work for camel tests #2451
Conversation
@smlambert Please review my PR for prep work for camel-tests. I have tried to do the task according to my understanding, but if anything was done wrong or anything needs to be changed, then please let me know and guide me. I will be happy to change it and learn more in the process. |
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.
Looking pretty good! I have asked for a few updates before a 2nd review occurs.
external/camel/README.md
Outdated
@@ -0,0 +1,20 @@ | |||
# External (Third Party Container) Tests |
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.
Please change the heading to:
External Camel Tests
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.
Done
external/camel/README.md
Outdated
# External (Third Party Container) Tests | ||
Third Party container tests help verify that the AdoptOpenJDK binaries are good by running a variety of Java applications inside of Docker containers. AdoptOpenJDK/openjdk-tests/Issue #172 lists the applications that we have initially targeted to best exercise the AdoptOpenJDK binaries. For each application, we choose to run a selection of their functional tests. | ||
|
||
## Running External tests locally |
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.
change to:
Running External Camel tests locally
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.
Done
external/camel/README.md
Outdated
|
||
## Running External tests locally | ||
To run any AQA tests locally, you follow the same pattern: | ||
Ensure your test machine is set up with test prereqs. For external tests, you do need Docker installed. |
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.
Please restore all of the hyperlinks and
special characters in this document so that </pathToWhereYouInstalledSDK>
does not dissappear due to being rendered as html. For the test prereqs hyperlink, if you can link to https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/doc/Prerequisites.md (instead of the one at the openj9 project), , it will be great!
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.
Done
external/camel/README.md
Outdated
@@ -0,0 +1,20 @@ | |||
# External (Third Party Container) Tests | |||
Third Party container tests help verify that the AdoptOpenJDK binaries are good by running a variety of Java applications inside of Docker containers. AdoptOpenJDK/openjdk-tests/Issue #172 lists the applications that we have initially targeted to best exercise the AdoptOpenJDK binaries. For each application, we choose to run a selection of their functional tests. |
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.
Please change the first part of the sentence to be:
Camel tests are part of the external third-party application tests that help verify that the Adoptium binaries are good...
and update references to AdoptOpenJDK to be Adoptium please.
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.
Done
external/camel/README.md
Outdated
8. `make compile` (This fetches test material and compiles it, based on build.xml files in the test directories) | ||
9. `make _camel_test` (When you defined BUILD_LIST to point to a directory in openjdk-tests/external, then this is a testCaseName from the playlist.xml file within the directory you chose) | ||
When [running these from the command-line](https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/doc/userGuide.md#local-testing-via-make-targets-on-the-commandline), these tests are grouped under a make target called 'external', so 'make external' would run the entire set of tests found in the openjdk-tests/external directory. This is unadvisable! Limit what you compile and run, BUILD_LIST=external/`<someSubDirectory>`, and TARGET=`<testCaseNameFromSubdirPlaylist>` | ||
These tests run regularly and results can be found in [TRSS Third Party Application view](https://trss.adoptopenjdk.net/ThirdPartyAppVie). |
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.
Missing 'w' at the end of the trss.adoptopenjdk link
external/camel/README.md
Outdated
@@ -0,0 +1,20 @@ | |||
# External Camel Tests | |||
Third Party container tests help verify that the Adoptium binaries are good by running a variety of Java applications inside of Docker containers. AdoptOpenJDK/openjdk-tests/Issue #172 lists the applications that we have initially targeted to best exercise the AdoptOpenJDK binaries. For each application, we choose to run a selection of their functional tests. |
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.
Can I get you to add an extra sentence after this one that says:
Camel tests are functional tests pulled from the camel-quarkus repository.
*please also hyperlink camel-quarkus using the value from github_url in test.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.
Done
external/camel/README.md
Outdated
@@ -0,0 +1,20 @@ | |||
# External Camel Tests | |||
Third Party container tests help verify that the Adoptium binaries are good by running a variety of Java applications inside of Docker containers. AdoptOpenJDK/openjdk-tests/Issue #172 lists the applications that we have initially targeted to best exercise the AdoptOpenJDK binaries. For each application, we choose to run a selection of their functional tests.Camel tests are functional tests pulled from the [camel-quarkus](https://github.com/apache/camel-quarkus.git) repository. |
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.
See comment #2451 (comment)
Once this is updated I think the PR is good to go.
7. export required environment variables, BUILD_LIST and EXTRA_DOCKER_ARGS (`export BUILD_LIST=external/camel` and `export EXTRA_DOCKER_ARGS="-v $TEST_JDK_HOME:/opt/java/openjdk"` | ||
8. `make compile` (This fetches test material and compiles it, based on build.xml files in the test directories) | ||
9. `make _camel_test` (When you defined BUILD_LIST to point to a directory in openjdk-tests/external, then this is a testCaseName from the playlist.xml file within the directory you chose) | ||
When [running these from the command-line](https://github.com/AdoptOpenJDK/openjdk-tests/blob/master/doc/userGuide.md#local-testing-via-make-targets-on-the-commandline), these tests are grouped under a make target called 'external', so 'make external' would run the entire set of tests found in the openjdk-tests/external directory. This is unadvisable! Limit what you compile and run, BUILD_LIST=external/`<someSubDirectory>`, and TARGET=`<testCaseNameFromSubdirPlaylist>` |
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.
Please add extra blank line before here, so it will be a new paragraph instead of part of number 9.
Please also sign off your commit and update your comment #2451 (comment) with Close #2429 , which will automatically close the issue #2229 when this PR is merged. |
@sophia-guo Could you please tell me what is meant by signing off commit? |
https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for Its not strictly required anymore (used to be require by Eclipse Foundation but effective March 30 has been dropped as a requirement). |
Signed-off-by: SAY-droid427 <[email protected]>
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
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.
thanks @SAY-droid427 !
Completed the prep work for camel tests
Close #2429