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 CryptoTests_jtreg #5240

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Add CryptoTests_jtreg #5240

merged 1 commit into from
Apr 19, 2024

Conversation

llxia
Copy link
Contributor

@llxia llxia commented Apr 18, 2024

related: #5206

@llxia llxia force-pushed the test2 branch 2 times, most recently from e673e88 to fc7d768 Compare April 18, 2024 18:37
@llxia
Copy link
Contributor Author

llxia commented Apr 18, 2024

# ================================================
# timestamp: 20240418-180952
# repo dir: CryptoTest
# git repo: 
#   Fetch URL: https://github.com/rh-openjdk/CryptoTest.git
# sha:
# 23496c5da6731eb7fbb65d49f212d2ca1ebbc751
# 

- add CryptoTests_jtreg back (based on the original change https://github.com/adoptium/aqa-tests/pull/5134/files)
- CryptoTests_jtreg directly triggers the tests via jtreg and it runs against all vendors except redhat
- CryptoTests only runs for vendor=redhat
- re-enable CryptoTests_jtreg on aarch64_mac|x86-64_mac|s390x_linux (related to run.sh issue. See CryptoTests readlink: illegal option -- f eclipse-openj9/openj9#19164)
- add retry for git clone to mitigate network issue
- add check CryptoTest repo sha (sha will be captured in TAP file)

Signed-off-by: Lan Xia <[email protected]>
Copy link
Contributor

@LongyuZhang LongyuZhang 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 @llxia !

@smlambert smlambert merged commit 140781d into adoptium:master Apr 19, 2024
2 checks passed
@@ -49,5 +49,47 @@
<groups>
<group>functional</group>
</groups>
<vendors>
<vendor>redhat</vendor>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be make opposite? To enable this main for all, except the few broken, and enable CryptoTests_jtreg only for the zOs or whereevere the main rnner do not fit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be make opposite? To enable this main for all, except the few broken, and enable CryptoTests_jtreg only for the zOs or whereevere the main rnner do not fit?

If this PR would have not moved the default exclusion in favour of itself it would be awesome temporary workaround for affected platforms before the real issue s fixed.

@judovana
Copy link
Contributor

To merging this without actually waiting for any input from original authors is really weird.

@smlambert
Copy link
Contributor

You provided your input @judovana. You said you need and want the run.sh wrapper for the way you run your Jenkins jobs, so you have a test target CryptoTest.

For all others who currently do not want the extra runner (or gain benefit from it) and who have been running this test material without it and are happy to do so get a test target called CryptoTest_jtreg.

Everyone gets to run the test material in the way they want and the committers do not have to keep milling around this topic when there are hundreds of other topics to attend to. The test material is invoked and running. This is the important aspect, that the tests are re-enabled and running.

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.

4 participants