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

Add Crypotests #4319

Closed
wants to merge 7 commits into from
Closed

Add Crypotests #4319

wants to merge 7 commits into from

Conversation

sophia-guo
Copy link
Contributor

@sophia-guo sophia-guo commented Feb 8, 2023

Add Crypotest #3829

  • Move existing security tests to sub dir
  • Update gitignore
  • Add task getDependentLibs to support compiling by subdir functional/security/AQAvit

Signed-off-by: Sophia Guo sophia.gwf@gmail.com

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@sophia-guo sophia-guo marked this pull request as draft February 8, 2023 05:04
Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@@ -0,0 +1,60 @@
<?xml version="1.0"?>

<!--
Copy link
Contributor

Choose a reason for hiding this comment

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

This material would not be Copyright IBM, but rather

<!--
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
-->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file was added by accident. Removed.

@@ -14,15 +14,15 @@
# limitations under the License.
-->

<project name="Security Functional tests" default="build" basedir=".">
<project name="Adotpium Security Functional tests" default="build" basedir=".">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<project name="Adotpium Security Functional tests" default="build" basedir=".">
<project name="Adoptium Security Functional tests" default="build" basedir=".">

Security Functional tests
</description>
<import file="${TEST_ROOT}/functional/build.xml"/>
Adotpium Security Functional tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Adotpium Security Functional tests
Adoptium Security Functional tests

@sophia-guo
Copy link
Contributor Author

CryptoTest will be added to functional security as a sub test. Would prefer to move the current security tests net.adoptopenjdk.test to sub directory. @ShelleyLambert is that ok? If it's ok any preference for the tests name? ( in my draft PR I just name it Adoptium by random).

@smlambert
Copy link
Contributor

Since this test material originates from rh-openjdk, then that would be an appropriate name to use.

@sophia-guo
Copy link
Contributor Author

sophia-guo commented Feb 8, 2023

@smlambert I'm talking the tests already exists in aqa-tests/functional/security, which was added by you. I'd like to move that one to a sub dir as one of security tests.

@smlambert
Copy link
Contributor

smlambert commented Feb 8, 2023

ah, yes, then aqavit is preferred name to use (since they originate as part of the AQAvit project), though adoptium could also work, as its the top-level project under which AQAvit, Temurin, and other sub-projects sit.

Screen Shot 2023-02-08 at 10 56 37 AM

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@sophia-guo
Copy link
Contributor Author

@sophia-guo sophia-guo marked this pull request as ready for review February 8, 2023 21:08
@sophia-guo sophia-guo requested review from smlambert and llxia February 8, 2023 21:09
Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
<src path="${src}" />
<classpath>
<pathelement location="${LIB_DIR}/testng.jar"/>
<pathelement location="${LIB_DIR}/jcommander.jar"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

testng.jar and jcommand.jar are used, but they are not set before line 23. This will result in failure for the local runs.

Please add the following line before line 23:

<property name="LIB" value="testng,jcommander" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default value of LIB is all. This should not fail the local run though it's alway good to limit to the lib needed.

@llxia
Copy link
Contributor

llxia commented Feb 8, 2023

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
Copy link
Contributor

@llxia llxia 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 @sophia-guo - there is a very recent upstream update to convert this test material for running with jtreg (see https://github.com/rh-openjdk/CryptoTest/pull/29/files, and cc-ed you on an email today). In light of that update, can you update this PR to try and use jtreg to launch the test material please?

@sophia-guo
Copy link
Contributor Author

Move to #4327 using jtreg

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

Successfully merging this pull request may close these issues.

3 participants