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 Arquillian Test Case for ElytroHttpClient #16761

Closed
wants to merge 2 commits into from

Conversation

keshav-725
Copy link

@keshav-725 keshav-725 commented Apr 20, 2023

@wildfly-ci
Copy link

Hello, keshav-725. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@jamezp jamezp added the rebase-this PR has a merge conflict. label Apr 21, 2023
@jamezp
Copy link
Member

jamezp commented Apr 21, 2023

Hi @keshav-725. We can't use SNAPSHOT dependencies, that said wildfly-core has been updated to 20.0.1.Final so hopefully the changes are in there.

@Skyllarr
Copy link
Contributor

@jamezp The changes are not yet in the wildfly-elytron or wildfly-core as this tests are for a feature.

@keshav-725 Can you please write into the description of this PR that it requires your elytron PR ? Thanks!

@bstansberry bstansberry added core-upgrade-needed PR requires a wildfly-core change to be merged and integrated first and removed rebase-this PR has a merge conflict. labels May 11, 2023
@bstansberry
Copy link
Contributor

@keshav-725 Please also file a WFLY JIRA for this and use the JIRA id in the commit message and in the title of this PR. Thanks! You can link the WFLY JIRA to whatever Elytron JIRA is being used to track what this is testing.

@keshav-725
Copy link
Author

@bstansberry I have updated the commit message as well as title of this PR.Also Linked the WFLY JIRA to ELYTRON JIRA.Thanks!

ElytronHttpClient elytronHttpClient = new ElytronHttpClient();
context.run(() -> {
try{
HttpResponse response = elytronHttpClient.connect(servletUrl.toString());
Copy link
Contributor

@Skyllarr Skyllarr Jun 12, 2023

Choose a reason for hiding this comment

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

@keshav-725 It would be good to add some tests that use the same elytronHttpClient instance for different requests in different AuthenticationContext? So something like:

ElytronHttpClient elytronHttpClient = new ElytronHttpClient();
..
AuthenticationContext context1 = ..
AuthenticationContext contex2 = ...
context.run() {...elytronHttpClient.send request..}
context2.run() {...elytronHttpClient.send request2..}

so we know that the elytronHttpClient works in changing authentication contexts. Some test could use a different URI for these requests as well. Let me know if you have any questions about that. Thank you!

@jamezp jamezp added the rebase-this PR has a merge conflict. label Jul 10, 2023
@bstansberry
Copy link
Contributor

@keshav-725 @Skyllarr @fjuma @darranl

I question the test strategy here. AFAICT this isn't testing WildFly at all, it's testing Elytron. It's just using WildFly as a test server. Can't this be done in the Elytron code base?

* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to use the current license header (see the other java files.)


import java.net.MalformedURLException;
import java.net.URL;
import java.net.http.HttpResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

The java.* imports come before the org.jboss ones.

import java.net.http.HttpResponse;

@RunWith(Arquillian.class)
public class DigestAuthnTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also have @RunAsClient.

<property name="serverConfig">${jboss.server.config.file.name:standalone.xml}</property>
<property name="jbossArguments">${jboss.args}</property>
<property name="allowConnectingToRunningServer">true</property>
<property name="managementAddress">${node0:127.0.0.1}</property>
<property name="managementPort">${as.managementPort:9990}</property>
<property name="javaVmArguments">${server.jvm.args} -Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=n</property>
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be removed.

@Skyllarr
Copy link
Contributor

Closing this PR for now as nobody is working on it currently

@Skyllarr Skyllarr closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-upgrade-needed PR requires a wildfly-core change to be merged and integrated first rebase-this PR has a merge conflict.
Projects
None yet
5 participants