KNOX-3077 - Add unit test for pac4j.cookie.max.age param#993
KNOX-3077 - Add unit test for pac4j.cookie.max.age param#993moresandeep merged 4 commits intomasterfrom
Conversation
...rity-pac4j/src/test/java/org/apache/knox/gateway/pac4j/filter/Pac4jDispatcherFilterTest.java
Outdated
Show resolved
Hide resolved
...rity-pac4j/src/test/java/org/apache/knox/gateway/pac4j/filter/Pac4jDispatcherFilterTest.java
Outdated
Show resolved
Hide resolved
...rity-pac4j/src/test/java/org/apache/knox/gateway/pac4j/filter/Pac4jDispatcherFilterTest.java
Outdated
Show resolved
Hide resolved
| List<String> params = new ArrayList<>(); | ||
| params.add(Pac4jDispatcherFilter.PAC4J_CALLBACK_URL); | ||
| params.add("clientName"); | ||
| params.add(SAML_KEYSTORE_PATH); | ||
| params.add(SAML_IDENTITY_PROVIDER_METADATA_PATH); |
There was a problem hiding this comment.
params are the same -> you could move it into setupCommonExpectations
There was a problem hiding this comment.
Agreed, but then that does not make the tests easy to read, it is like database normalization, yes we will achieve less lines of code but at the cost of readability and we won't get any performance out of it either since this will be anyways run by JUnit framework for each tests. This was my argument against restructuring the test code in the first review, i like to see all the expectations and mock objects in the function so that they are easy to read and update in the future. Consider when we have to update the tests now it is not as easy to modify these multiple functions that could affect other tests. Also, creating new test cases might be a bit more work as we have to go though all the functions to understand what it does. Basically, my point is keeping all the code in one function is simpler to read and extend, there is no performance impact other than taking up some extra space on github, also the ROI for these changes is really low :)
my 2 cents :)
smolnar82
left a comment
smolnar82
left a comment
There was a problem hiding this comment.
LGTM :)
Thanks for addressing my comment so quickly.
Awesome! thank you for the detailed comments, as always greatly appreciate your feedback !! |
3 participants
What changes were proposed in this pull request?
This PR adds a unit test for pac4j.cookie.max.age param
How was this patch tested?
Patch was tested locally.