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

[Part 1] Replace JUnit assertEquals() with Hamcrest matchers assertThat() #3500

Closed
wants to merge 25 commits into from

Conversation

EduardoCorazon
Copy link
Contributor

@EduardoCorazon EduardoCorazon commented Oct 8, 2023

Description

This is a new PR based on: #3443

This change is a response to opensearch-project/security issue #1832 whereby it's recommended to replace Assert.assertEquals(...) with assertThat(...) for a greater degree of verbosity.

Upon collaboration, this PR will try to address and refactor files to use assertThat() as prompted in #3443 (review)

  • Refactoring
  • Using the Hamcrest matchers with assertThat() yields clearer failure messages for testing.
  • The old behavior uses assertEquals() which is part of the Assert class in JUnit meanwhile the new method, assertThat(), belongs to the Hamcrest class which tends to be more human readable.

Issues Resolved

Is this a backport? If so, please add backport PR # and/or commits #
backport 2.x

Testing

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…iTest.java

Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
@EduardoCorazon EduardoCorazon changed the title [New] Replaced Assert.assertEquals() with assertThat() on AllowlistAp… [New] Response to Issue #1832 [Replace Assert.assertEquals() with assertThat()] Oct 8, 2023
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #3500 (94e479d) into main (4676452) will increase coverage by 64.89%.
Report is 11 commits behind head on main.
The diff coverage is n/a.

❗ Current head 94e479d differs from pull request most recent head 03cf614. Consider uploading reports for the commit 03cf614 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3500       +/-   ##
===========================================
+ Coverage        0   64.89%   +64.89%     
- Complexity      0     3640     +3640     
===========================================
  Files           0      284      +284     
  Lines           0    20619    +20619     
  Branches        0     3391     +3391     
===========================================
+ Hits            0    13381    +13381     
- Misses          0     5553     +5553     
- Partials        0     1685     +1685     

see 284 files with indirect coverage changes

@DarshitChanpura
Copy link
Member

Screenshot 2023-10-09 at 11 11 28 AM

@EduardoCorazon Seems like there are lot more occurrences for assertEquals

@EduardoCorazon
Copy link
Contributor Author

@DarshitChanpura I will definitely get to working on them! I just made this new PR to fix some things on my end. I want to make sure that each full file replacement of assertEquals() works before working on the next. I already have two more commits to push but I'm trying to figure out why this simple replacement is failing CI tests.

Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
@DarshitChanpura
Copy link
Member

@EduardoCorazon The backwards compatibilty tests will pass once #3498 is merged

@DarshitChanpura DarshitChanpura changed the title [New] Response to Issue #1832 [Replace Assert.assertEquals() with assertThat()] Replace JUnit assertEquals() with Hamcrest matchers assertThat() Oct 10, 2023
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
@DarshitChanpura DarshitChanpura marked this pull request as draft October 13, 2023 15:23
@DarshitChanpura
Copy link
Member

@EduardoCorazon I've marked this as draft. Please mark it as ready for review once all changes are complete.

Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
@EduardoCorazon
Copy link
Contributor Author

I apologize for the delayed action on this draft. I can't believe it did not occur to me to just use regular expressions 😅

Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
@DarshitChanpura
Copy link
Member

@EduardoCorazon What do you think about splitting this PR into two parts? I see that you are halfway there with about 80 files left. This would allow the changes to be manageable while reviewing.

@EduardoCorazon
Copy link
Contributor Author

@DarshitChanpura that's a great idea! Let me quickly push this last change before splitting.

Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
@EduardoCorazon
Copy link
Contributor Author

ready to go

@EduardoCorazon EduardoCorazon changed the title Replace JUnit assertEquals() with Hamcrest matchers assertThat() [Part 1] Replace JUnit assertEquals() with Hamcrest matchers assertThat() Nov 1, 2023
@DarshitChanpura
Copy link
Member

@EduardoCorazon There are some outstanding file conflicts. I'll review once you address them

EduardoCorazon and others added 10 commits November 1, 2023 20:40
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
git commit -m "merge newest main with Dev to fix conflicts" -s
Signed-off-by: Eduardo Corazon <corazoneduardo@att.net>
@EduardoCorazon
Copy link
Contributor Author

EduardoCorazon commented Nov 12, 2023

@DarshitChanpura After talking with Stephen it seems that separating it into parts might be causing the issues since the changes affect important files. As such, a possible solution would be to submit all changes in a single PR but this would make reviewing changes a bit tedious. However, I don't think I will have enough time to finish replacing the remaining files before the end of the program. I deeply apologize for the length of time I've taken to address this issue and for my lack of efficiency in such an approach. I've submitted a new issue with a little bit of notes and recommendations since the original issue is from 2022. If possible, I will try my best to keep contributing to the repo even after the end of the program if that's ok. Again I deeply apologize. However, if necessary, I would be more than happy to re-open and finish this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants