ELY-2641 : Simplify assersions in MaskCommandTest#testMissingSaltAnd iteration method #1961

nidhiazad · 2023-09-22T17:49:31Z

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

Simplify assersions in MaskCommandTest#testMissingSaltAndIteration method

PrarthonaPaul · 2023-09-22T17:59:42Z

Thank you @nidhiazad for the PR!
@Skyllarr just wondering, does this line also need to be turned to assertEquals?
assertTrue("output has to be the as pre-generated one", ("MASK-" + pbGenerated + ";" + "ASDF1234" + ";" + 123).equals(retValNoNewLine));
If not, then it looks good to me

SoniaZaldana · 2023-09-22T18:02:03Z

Thank you @nidhiazad for the PR! @Skyllarr just wondering, does this line also need to be turned to assertEquals? assertTrue("output has to be the as pre-generated one", ("MASK-" + pbGenerated + ";" + "ASDF1234" + ";" + 123).equals(retValNoNewLine)); If not, then it looks good to me

@PrarthonaPaul I think the issue only involves changing statements in testMissingSaltAndIteration. The line you quoted is within another test, so most likely not :)

PrarthonaPaul · 2023-09-22T18:02:58Z

Thank you @nidhiazad for the PR! @Skyllarr just wondering, does this line also need to be turned to assertEquals? assertTrue("output has to be the as pre-generated one", ("MASK-" + pbGenerated + ";" + "ASDF1234" + ";" + 123).equals(retValNoNewLine)); If not, then it looks good to me

@PrarthonaPaul I think the issue only involves changing statements in testMissingSaltAndIteration. The line you quoted is within another test, so most likely not :)

Awesome, thank you @SoniaZaldana
In that case, this PR looks good! Thank you @nidhiazad

SoniaZaldana

Looks good to me!

ashley-abdelsayed98

thanks for your contribution @nidhiazad

ashley-abdelsayed98 · 2023-09-22T18:06:43Z

tool/src/test/java/org/wildfly/security/tool/MaskCommandTest.java

-        assertTrue("Message about invalid iteration parameter must be present", ("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.").equals(retValLines[1]));
-        assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));
+        assertEquals("Message about invalid salt parameter must be present", true, retValLines[0].contains("Invalid \"salt\" parameter. Generated value"));
+        assertEquals("Message about invalid iteration parameter must be present", true, ("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.").equals(retValLines[1]));


this statement is passing true as the expected value and then passes this line:("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.").equals(retValLines[1]) as the actual value to the assertEquals function to confirm that they are equal. we can simplify this by just passing ("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.") as the expected value and retValLines[1] as the actual value to the assertEquals function and the function will compare to check if the 2 strings are equal.

okay, thanks. Fixing this.

ashley-abdelsayed98 · 2023-09-22T18:07:43Z

tool/src/test/java/org/wildfly/security/tool/MaskCommandTest.java

-        assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));
+        assertEquals("Message about invalid salt parameter must be present", true, retValLines[0].contains("Invalid \"salt\" parameter. Generated value"));
+        assertEquals("Message about invalid iteration parameter must be present", true, ("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.").equals(retValLines[1]));
+        assertEquals("Message about invalid salt parameter must be present",true, retValLines[2].contains("MASK-"));


same as above, i think we can keep the original statement that asserts retValLines[2].contains("MASK-") is true, rather than asserting that statement is equal to true

thanks @nidhiazad. rather than using assertEquals for all 3 statements, i think the first and last statement can be kept with assertTrue, instead of asserting that retValLines[2].contains("MASK-") is true

Suggested change

assertEquals("Message about invalid salt parameter must be present",true, retValLines[2].contains("MASK-"));

assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));

ashley-abdelsayed98 · 2023-09-22T18:08:01Z

tool/src/test/java/org/wildfly/security/tool/MaskCommandTest.java

-        assertTrue("Message about invalid salt parameter must be present", retValLines[0].contains("Invalid \"salt\" parameter. Generated value"));
-        assertTrue("Message about invalid iteration parameter must be present", ("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.").equals(retValLines[1]));
-        assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));
+        assertEquals("Message about invalid salt parameter must be present", true, retValLines[0].contains("Invalid \"salt\" parameter. Generated value"));


i think we can keep the original statement that asserts retValLines[0].contains("Invalid \"salt\" parameter. Generated value") is true, rather than asserting that statement is equal to true

nidhiazad · 2023-09-22T18:31:05Z

updated the code. Please review and let me know if anything needs to be done.

ashley-abdelsayed98 · 2023-09-22T18:34:56Z

tool/src/test/java/org/wildfly/security/tool/MaskCommandTest.java

-        assertTrue("Message about invalid salt parameter must be present", retValLines[0].contains("Invalid \"salt\" parameter. Generated value"));
-        assertTrue("Message about invalid iteration parameter must be present", ("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.").equals(retValLines[1]));
-        assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));
+        assertEquals("Invalid \"salt\" parameter. Generated value", retValLines[0]);


for this case, we want to keep the original statement that retValLines[0] contains the string "Invalid \"salt\" parameter. Generated value" but it is not equal to it

ashley-abdelsayed98 · 2023-09-22T18:37:15Z

tool/src/test/java/org/wildfly/security/tool/MaskCommandTest.java

-        assertTrue("Message about invalid iteration parameter must be present", ("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.").equals(retValLines[1]));
-        assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));
+        assertEquals("Invalid \"salt\" parameter. Generated value", retValLines[0]);
+        assertEquals("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.",retValLines[1]);


for all 3 statements, we can keep the original messages that indicaet why this assertion should pass:

Suggested change

assertEquals("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.",retValLines[1]);

assertEquals("Message about invalid salt parameter must be present", "Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.",retValLines[1]);

@ashley-abdelsayed98 I am passing message,expected,actual in the assertEquals method for all the three statements now. This is how it needs to be done. Sorry about the previous commits. Hope so this one is correct. Please let me know if any alteration is required.

Skyllarr · 2023-09-22T19:04:51Z

Thank you @nidhiazad for the PR! @Skyllarr just wondering, does this line also need to be turned to assertEquals? assertTrue("output has to be the as pre-generated one", ("MASK-" + pbGenerated + ";" + "ASDF1234" + ";" + 123).equals(retValNoNewLine)); If not, then it looks good to me

@PrarthonaPaul I think the issue only involves changing statements in testMissingSaltAndIteration. The line you quoted is within another test, so most likely not :)

That's right! This

Thank you @nidhiazad for the PR! @Skyllarr just wondering, does this line also need to be turned to assertEquals? assertTrue("output has to be the as pre-generated one", ("MASK-" + pbGenerated + ";" + "ASDF1234" + ";" + 123).equals(retValNoNewLine)); If not, then it looks good to me

@PrarthonaPaul I think the issue only involves changing statements in testMissingSaltAndIteration. The line you quoted is within another test, so most likely not :)

Yes this test is specifically for the method testMissingSaltAndIteration. IIRC we have other jira issues that address this in other methods

ashley-abdelsayed98

thanks for the changes @nidhiazad !

ashley-abdelsayed98 · 2023-09-22T19:16:08Z

tool/src/test/java/org/wildfly/security/tool/MaskCommandTest.java

-        assertTrue("Message about invalid salt parameter must be present", retValLines[0].contains("Invalid \"salt\" parameter. Generated value"));
-        assertTrue("Message about invalid iteration parameter must be present", ("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.").equals(retValLines[1]));
-        assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));
+        assertEquals("Message about invalid salt parameter must be present","Invalid \"salt\" parameter. Generated value", retValLines[0]);


i think this test will still fail because retValLines[0] does not equal "Invalid \"salt\" parameter. Generated value", but rather it just contains it as a substring

ashley-abdelsayed98 · 2023-09-22T19:16:39Z

tool/src/test/java/org/wildfly/security/tool/MaskCommandTest.java

-        assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));
+        assertEquals("Message about invalid salt parameter must be present","Invalid \"salt\" parameter. Generated value", retValLines[0]);
+        assertEquals("Message about invalid iteration parameter must be present","Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.",retValLines[1]);
+        assertEquals("Message about invalid salt parameter must be present","MASK-",retValLines[2]);


same as above, i think this test will fail because retValLines[2] does not equal "MASK-", but rather it just contains it as a substring

I have submitted the new changes and verified by running mvn clean install -Dtest=MaskCommandTest. The test is passing now.

Skyllarr · 2023-09-22T19:19:00Z

@nidhiazad Just a minor, in the testMissingSaltAndIteration is a single assertTrue that can be updated to use assertEquals easily.
The methods that use assertTrue(object1.equals(object2)) can be updated to use assertEquals(object1, object2). The other 2 assertions use the contains() method. The values in the contains methods are not written in full, so the assertEquals cannot be used there as @ashley-abdelsayed98 explained above.

You can try to run the project with mvn clean install and you will see that this test will not pass. Alternatively, you can change to the tool subdirectory of the project and run mvn clean install -Dtest=MaskCommandTest#testMissingSaltAndIteration which will run only this single test .

SoniaZaldana

Hi @nidhiazad, thanks for the changes. They look good!

Could you please squash your commits? Let me know if you need any help with that.

Skyllarr · 2023-09-22T21:32:28Z

@nidhiazad The changes look great now! It just need squashing of the commits into a single commit. If you need any help, please let us know

…teration method

ashley-abdelsayed98

LGTM, thank you @nidhiazad!

Skyllarr · 2023-09-22T22:23:23Z

@nidhiazad Thank you!

nidhiazad requested review from fjuma and Skyllarr as code owners September 22, 2023 17:49

SoniaZaldana approved these changes Sep 22, 2023

View reviewed changes

ashley-abdelsayed98 reviewed Sep 22, 2023

View reviewed changes

SoniaZaldana approved these changes Sep 22, 2023

View reviewed changes

SoniaZaldana suggested changes Sep 22, 2023

View reviewed changes

ELY-2641 : Simplify assersions in MaskCommandTest#testMissingSaltAndI…

b9bf6dc

…teration method

nidhiazad force-pushed the feature/ELY-2641_OSD_Nidhi_Azad branch from 3d0d312 to b9bf6dc Compare September 22, 2023 21:59

Skyllarr approved these changes Sep 22, 2023

View reviewed changes

ashley-abdelsayed98 approved these changes Sep 22, 2023

View reviewed changes

Skyllarr merged commit 174424c into wildfly-security:2.x Sep 22, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ELY-2641 : Simplify assersions in MaskCommandTest#testMissingSaltAnd iteration method #1961

ELY-2641 : Simplify assersions in MaskCommandTest#testMissingSaltAnd iteration method #1961

nidhiazad commented Sep 22, 2023

PrarthonaPaul commented Sep 22, 2023

SoniaZaldana commented Sep 22, 2023

PrarthonaPaul commented Sep 22, 2023

SoniaZaldana left a comment

ashley-abdelsayed98 left a comment

ashley-abdelsayed98 Sep 22, 2023

nidhiazad Sep 22, 2023

ashley-abdelsayed98 Sep 22, 2023

ashley-abdelsayed98 Sep 22, 2023

ashley-abdelsayed98 Sep 22, 2023

nidhiazad commented Sep 22, 2023

ashley-abdelsayed98 Sep 22, 2023

ashley-abdelsayed98 Sep 22, 2023

nidhiazad Sep 22, 2023

Skyllarr commented Sep 22, 2023

ashley-abdelsayed98 left a comment

ashley-abdelsayed98 Sep 22, 2023

ashley-abdelsayed98 Sep 22, 2023

nidhiazad Sep 22, 2023

Skyllarr commented Sep 22, 2023 •

edited

Loading

SoniaZaldana left a comment

Skyllarr commented Sep 22, 2023

ashley-abdelsayed98 left a comment

Skyllarr commented Sep 22, 2023

	assertEquals("Message about invalid salt parameter must be present",true, retValLines[2].contains("MASK-"));
	assertTrue("Message about invalid salt parameter must be present", retValLines[2].contains("MASK-"));

	assertEquals("Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.",retValLines[1]);
	assertEquals("Message about invalid salt parameter must be present", "Invalid \"iteration\" parameter. Default value \"" + defaultIteration + "\" will be used.",retValLines[1]);

ELY-2641 : Simplify assersions in MaskCommandTest#testMissingSaltAnd iteration method #1961

ELY-2641 : Simplify assersions in MaskCommandTest#testMissingSaltAnd iteration method #1961

Conversation

nidhiazad commented Sep 22, 2023

PrarthonaPaul commented Sep 22, 2023

SoniaZaldana commented Sep 22, 2023

PrarthonaPaul commented Sep 22, 2023

SoniaZaldana left a comment

Choose a reason for hiding this comment

ashley-abdelsayed98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nidhiazad commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr commented Sep 22, 2023

ashley-abdelsayed98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr commented Sep 22, 2023 • edited Loading

SoniaZaldana left a comment

Choose a reason for hiding this comment

Skyllarr commented Sep 22, 2023

ashley-abdelsayed98 left a comment

Choose a reason for hiding this comment

Skyllarr commented Sep 22, 2023

Skyllarr commented Sep 22, 2023 •

edited

Loading