-
-
Notifications
You must be signed in to change notification settings - Fork 27
Add equals and hashcode #168
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
base: master
Are you sure you want to change the base?
Conversation
Thank you for the PR. Would it possible to include also unit tests to check the different branches with the equals and hashcode? |
keyValuePairs, | ||
markers); | ||
|
||
assertThat(logEventA).isEqualTo(logEventB); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it is obvious that these two will be equal, however your use case was different right? You wanted to get the logs from logcaptor and use the collection api to check whether it contains a logevent. Would it be possible for you to use logEventA from LogCaptor and keep logEventB as is so you can use the contains method from the collections to check whether logEventB contains in the logEvents?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not used to writing tests for equals and hashcode.
I have been taught to never implement these methods myself, but instead have my IDE (intellij) to generate the implementations.
And writing a test that verifies the functionality of the contains-method of java does not really seem to be our responsibility either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the unit test should be similar to your use case right? You mentioned that you want to use the a logEvent or logMarker to check whether that contains in the logCaptor, so I would image something like this:
LogMarker expectedLogMarker = new LogMarker("some-marker-name", Collections.emptyList());
logCaptor.getLogEvents().stream().anyMatch(event -> event.getMarkers().contains(expectedLogMarker));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got some time to spare to write such a test.
Is there anything else you see this PR is missing?
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I was silent for quite some time as I needed time to think about why we need this kind of solution and what it is attempting to solve. Looking to the code changes you made a was more convinced that this is not the solution I would prefer for the usecase you mentioned. You mentioned having the equals and hashcode method for logevent and logmarker would make it for you easier to check whether a list contains a specific element in the captured logevents or logmarkers. With this solution you always need to prepare an expected logevent or logmarker which almost impossible especially a logevent with a timestamp will never match with the actual one. Although you did create an unit test, it does not reflect a real use case. It would make asserting anything returned from logcaptor complex, so I want to propose a different solution. You basically want to check whether logcaptor has a specifc marker, I would suggest the following code snippet to achieve that: assertThat(logCaptor.getLogEvents())
.flatExtracting(LogEvent::getMarkers)
.anyMatch(logMarker -> "REF_MARKER".equals(logMarker.getName())); Or asserting something on the logevenets: assertThat(logCaptor.getLogEvents())
.anyMatch(logEvent -> "some log message".equals(logEvent.getMessage())); Or validating multiple fields in logevent and logmarker: assertThat(logCaptor.getLogEvents())
.anyMatch(logEvent -> "some log message".equals(logEvent.getMessage())
&& logEvent.getThrowable().isPresent()
&& logEvent.getLevel().equalsIgnoreCase("debug"))
.flatExtracting(LogEvent::getMarkers)
.anyMatch(logMarker -> "REF_MARKER".equals(logMarker.getName())); With this setup you don't need to prepare an expect object as logevent or marker. You don't need to have complext streaming setup to iterate through a list. I would suggest to use AssertJ library to handle these kind of usecases. What do you think, would this alternative also work for you? |
Of course, that solution would work, or so would even reflection do to get the actual slf4j loggingevents. The LogMarker and LogEvent objects are currently inheriting the equals and hashcode methods from their parent, Object, as any instance of a class would do in java. I find it reasonable to override the inherited version of these methods on objects used as data holders, such as on a Customer-class, Order-class or LogEvent and LogMarker-class, as they all are used to hold data of some sort and not control a flow. Your suggested solution would work no matter if equals or hashcode are implemented, but mine would not, as the inherited version of equals and hashcode does not represent when two LogMarker are equal, and that is a shame, I think. |
I have an idea that might be usable for others, and work around your finding regarind the preparation setup. I´ll push it to this branch as well, in a bit. |
By adding equals and hashcode methods to model classes, we can use the methods provided via the Collection-api, such as .contains(). If users has a need to handle model objects in Maps, such as HashMap, this is now also better supported.
Also renaming MarkerMapper to LogMarkerMapper for naming consistency.