-
-
Notifications
You must be signed in to change notification settings - Fork 320
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 Authorization Code Grant OAuth security test #2970
Conversation
system/security/playlist.xml
Outdated
To avoid test target duplication, this belongs to sanity, extended and special. Regular test should only belong to one level --> | ||
<test> | ||
<testCaseName>OAuthTest</testCaseName> | ||
<command>$(SYSTEMTEST_CMD_TEMPLATE) -test=OAuthTest</command> |
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.
<command>$(SYSTEMTEST_CMD_TEMPLATE) -test=OAuthTest</command> | |
<command>$(SYSTEMTEST_CMD_TEMPLATE) -test=OAuthTest; $(TEST_STATUS)</command> |
The special target to get machine information probably need to be added. Similar to https://github.com/adoptium/aqa-tests/blob/master/system/daaLoadTest/playlist.xml#L4-L19 |
system/security/playlist.xml
Outdated
<testCaseName>OAuthTest</testCaseName> | ||
<command>$(SYSTEMTEST_CMD_TEMPLATE) -test=OAuthTest; $(TEST_STATUS)</command> | ||
<levels> | ||
<level>sanity</level> |
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.
Regular test should only belong to one level. Which level would this OAuthTest belong to?
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.
My preference will be to add it to <level>extended</level>
only.
system/security/playlist.xml
Outdated
<command>$(JAVA_COMMAND) -cp $(JVM_TEST_ROOT)$(D)TKG$(D)bin$(D)TestKitGen.jar org.openj9.envInfo.EnvDetector MachineInfo; \ | ||
$(TEST_STATUS)</command> | ||
<levels> | ||
<level>extended</level> |
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.
We'd like this special target belongs to sanity, extended and special.
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.
Yes we should explain to @joeyleeeeeee97 - MachineInfo, unlike other targets should belong to all 3 levels of testing, so that it gets run in each test job generated for that level (it acts as a placeholder in the event that the only other test target in a test run is disabled or skipped on a platform, it will get run and allows the Jenkins job to have some test report to share and indicate that 'all is well' / status green).
hi @joeyleeeeeee97 - just looking for one last update to have MachineInfo target be tagged in all 3 levels (sanity, extended, special) and then this is ready to merge (along with your PR in aqa-systemtest repo). |
Hi. please have another look |
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, thanks @joeyleeeeeee97 !
This needs to be merged after adoptium/aqa-systemtest#457 is merged and after we tag the repos with the v1.0.0 tag. |
Now that most October release activities are done, can merge this with its associated aqa-systemtest PR. |
Related to adoptium/aqa-systemtest#453