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

[ELY-2171] Add test for MappedRegexRealmMapper #1601

Merged
merged 1 commit into from Oct 20, 2021

Conversation

pedro-hos
Copy link
Contributor

https://issues.redhat.com/browse/ELY-2171

I hope to create the test on the correct project and package. Also, let me if there are some specific regex and values to test.

@wildfly-ci
Copy link

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

/**
* @author <a href="mailto:pesilva@redhat.com">Pedro Silva</a>
*/
public class MappedRegexRealmMapperTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pedro-hos Thank you for PR! Just a minor, we do have many tests in the tests module but since we modularized the project we are trying to place the tests to the appropriate module. So this test can be placed in wildfly-elytron-auth-util since the MappedRegexRealmMapper is there.


@Test
public void shouldReturnNullForNonNamePrincipalInstance() {
Map<String, String> realmNameMap = Map.of();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the Map.of() method here and in other tests. It is a method documented as since jdk 9. We do use 11 for development but target platform can be 8. This is just a test method so maybe it is ok. @fjuma WDYT?

@Skyllarr
Copy link
Contributor

Skyllarr commented Oct 15, 2021

@pedro-hos let me know if there are some specific regex and values to test. - The tests you added are sufficient to me.

@pedro-hos pedro-hos force-pushed the ELY-2171 branch 2 times, most recently from 97a61bc to 8b9eef8 Compare October 15, 2021 14:05
@pedro-hos
Copy link
Contributor Author

@Skyllarr Thank you for your feedback! I've made the changes. I replace the class for the respective project module, and replace the Map.of() to Collections.emptyMap();.

~ See the License for the specific language governing permissions and
~ limitations under the License.
-->
<!-- ~ JBoss, Home of Professional Open Source. ~ Copyright 2018 Red Hat,
Copy link
Contributor

Choose a reason for hiding this comment

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

@pedro-hos Thanks for the changes! Just a minor - the formatting of pom.xml file now seems to be off.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be you used tabs here, and the project uses spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I changed the java editor configuration to spaces and forgot to replace it on other editors.

pedro-hos added a commit to pedro-hos/wildfly-elytron that referenced this pull request Oct 15, 2021
@@ -18,8 +18,8 @@
-->

<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
Copy link
Contributor

Choose a reason for hiding this comment

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

@pedro-hos Thank you for update. Just a minor but these 2 lines are still diferently formatted than in other pom files. Otherwise LGMT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Skyllarr sure, thanks

@Skyllarr
Copy link
Contributor

@pedro-hos Another minor detail, can you please squash the 2 commits together? Thank you.

pedro-hos added a commit to pedro-hos/wildfly-elytron that referenced this pull request Oct 18, 2021
[ELY-2171] Add test for MappedRegexRealmMapper

[ELY-2171] Add test for MappedRegexRealmMapper

[ELY-2171] Add test for MappedRegexRealmMapper wildfly-security#1601

[ELY-2171] Add test for MappedRegexRealmMapper
@@ -0,0 +1,63 @@
/*
* JBoss, Home of Professional Open Source.
* Copyright 2016 Red Hat, Inc., and individual contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

2021 :)

public class MappedRegexRealmMapperTest {

@Test
public void shouldReturnNullForNonNamePrincipalInstance() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically use "test" instead "should"

@@ -39,15 +39,15 @@
<dependency>
<groupId>org.wildfly.security</groupId>
<artifactId>wildfly-elytron-auth</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just minor but looks like there are still some whitespace changes in this file and the next one.

pedro-hos added a commit to pedro-hos/wildfly-elytron that referenced this pull request Oct 18, 2021
[ELY-2171] Add test for MappedRegexRealmMapper

[ELY-2171] Add test for MappedRegexRealmMapper

[ELY-2171] Add test for MappedRegexRealmMapper wildfly-security#1601

[ELY-2171] Add test for MappedRegexRealmMapper

Add test for MappedRegexRealmMapper
@Skyllarr
Copy link
Contributor

@pedro-hos This needs rebase now.

[ELY-2171] Add test for MappedRegexRealmMapper

[ELY-2171] Add test for MappedRegexRealmMapper

[ELY-2171] Add test for MappedRegexRealmMapper wildfly-security#1601

[ELY-2171] Add test for MappedRegexRealmMapper

Add test for MappedRegexRealmMapper
@@ -60,17 +60,17 @@
<groupId>org.wildfly.security</groupId>
<artifactId>wildfly-elytron-security-manager-action</artifactId>
</dependency>

Copy link
Contributor

Choose a reason for hiding this comment

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

@fjuma I checked our other pom files and we do sometimes have these empty lines with spaces and sometimes not. So I think whitespace changes in this pom are ok.

@Skyllarr Skyllarr added the +1 DV label Oct 20, 2021
Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

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

Thanks @pedro-hos!

@fjuma fjuma merged commit c348b9e into wildfly-security:1.x Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants