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

Added a README.md & test.properties file to the wycheproof docs #2463

Merged
merged 3 commits into from
Apr 9, 2021

Conversation

Annysah
Copy link
Contributor

@Annysah Annysah commented Apr 6, 2021

No description provided.

@Annysah
Copy link
Contributor Author

Annysah commented Apr 6, 2021

@karianna @smlambert Kindly review my PR.

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

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

hi @Annysah - some updates requested.

Comment on lines 467 to 481
wycheproof)
github_url="https://github.com/google/wycheproof.git"
script="wycheproof.sh"
home_path="\${WORKDIR}"
test_results="testResults"
tag_version="master"
bazel_version="1.2.1"
debian_packages="git wget unzip zip g++"
debianslim_packages="${debian_packages}"
ubuntu_packages="${debian_packages}"
alpine_packages="bash git wget unzip zip g++ bash"
centos_packages="git wget unzip zip gcc-c++"
clefos_packages="${centos_packages}"
ubi_packages="git wget unzip zip gcc-c++"
ubi_minimal_packages="${ubi_packages}"
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need this section for the test.properties. Take a look at https://github.com/AdoptOpenJDK/openjdk-tests/pull/2452/files for a similar example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @smlambert for the review, working on the updates requested.

@@ -0,0 +1,14 @@
## Running External tests locally
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a Heading and paragraph (similar to https://github.com/AdoptOpenJDK/openjdk-tests/pull/2452/files):

External Wycheproof Tests

Wycheproof tests are a part of the external third-party application tests that 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 Adoptium binaries. For each application, we choose to run a selection of their functional tests. Wycheproof test material is pulled from the wycheproof repository.

4. `cd openjdk-tests`
5. `./get.sh`
6. `cd TKG`
7. export required environment variables, BUILD_LIST and EXTRA_DOCKER_ARGS (example: `export BUILD_LIST=external/jacoco` and `export EXTRA_DOCKER_ARGS="-v $TEST_JDK_HOME:/opt/java/openjdk"`
Copy link
Contributor

Choose a reason for hiding this comment

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

BUILD_LIST=external/wycheproof

6. `cd TKG`
7. export required environment variables, BUILD_LIST and EXTRA_DOCKER_ARGS (example: `export BUILD_LIST=external/jacoco` and `export EXTRA_DOCKER_ARGS="-v $TEST_JDK_HOME:/opt/java/openjdk"`
8. `make wycheproof` (This fetches test material and compiles it, based on build.xml files in the test directories)
9. `make wycheproof_test` (When you defined BUILD_LIST to point to a directory in [openjdk-tests/external](https://github.com/AdoptOpenJDK/openjdk-tests/tree/master/external), then this is a testCaseName from the playlist.xml file within the directory you chose)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add an extra paragraph at the end of the README:

When running these from the command-line, 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.

@karianna karianna added this to the April 2021 milestone Apr 7, 2021
@Annysah
Copy link
Contributor Author

Annysah commented Apr 8, 2021

@smlambert The requested updates has been made, please help review my PR


9. `make wycheproof_test` (When you defined BUILD_LIST to point to a directory in [openjdk-tests/external](https://github.com/AdoptOpenJDK/openjdk-tests/tree/master/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>
Copy link
Contributor

@sophia-guo sophia-guo Apr 9, 2021

Choose a reason for hiding this comment

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

The last sentence we can use insert the code to make someSubDirectory or testCaseNameFromSubdirPlaylist visible. So BUILD_LIST=external/<someSubDirectory>, and TARGET=<testCaseNameFromSubdirPlaylist>.

And I would suggest to update as BUILD_LIST=external/wycheproof, and TARGET=<testCaseNameFromPlaylistUnderDirwycheproof>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sophia-guo for pointing this out, making the changes ASAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the wycheproof in this sentence be in angle brackets as well like you did for "" BUILD_LIST=external/wycheproof, and TARGET=

Copy link
Contributor

Choose a reason for hiding this comment

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

As it is a specific subdirectory so i think no need to put the angle brackets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sophia-guo, The required changes has been made, kindly review.

Copy link
Contributor

@sophia-guo sophia-guo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

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

thanks @Annysah !

@Annysah
Copy link
Contributor Author

Annysah commented Apr 9, 2021

Yaay!
Thanks @smlambert & @sophia-guo

@smlambert smlambert merged commit 36cd6eb into adoptium:master Apr 9, 2021
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.

4 participants