CODEC-285-JUnit5#113
Conversation
kinow
left a comment
There was a problem hiding this comment.
Hi @itamarc (olá! 👋 )
You'll have to rebase your branch, looks like there are some conflicts now 👍 After that we will likely squash the commits down to a single commit or maybe a couple meaningful commits (it'd be better if you could do it).
Thanks!
Bruno
|
Hi, @kinow (olá) I can work on solving the conflicts and also squash the commits to one, but...
(there are a "Resolve conflicts" button, but it's disabled for me) How can I see the conflicts so I can solve them?
Sorry, it's my first PR of this size. And thank you in advance for your help. Itamar |
|
You can fetch the master and then rebase on that. On the command line: The rebase attempts to collect all your commits and apply them sequentially to the master. If this is not possible the rebase will warn about the conflicts. You then fix those locally and continue with the rebase, IIRC Eventually you should end up with a set of changes that can be applied directly onto the master branch. You then force push the changes to this PR branch to update it: |
| } catch (final EncoderException ee) { | ||
| // An exception should be thrown | ||
| } | ||
| assertThrows(EncoderException.class, () -> { encoder.encode(null); }); |
There was a problem hiding this comment.
@itamarc
Thank you for your PR.
You do not need the curly brackets here, less clutter is better.
| } | ||
| Assert.assertTrue("An exception was not thrown when we tried to encode " + "a Float object", exceptionThrown); | ||
| final StringEncoder encoder = this.getStringEncoder(); | ||
| assertThrows(EncoderException.class, () -> { encoder.encode(Float.valueOf(3.4f)); }, |
There was a problem hiding this comment.
No curlies needed; I'll assume you'll go over the other files. I'll review again after you push.
There was a problem hiding this comment.
I tried to not change or remove any of the messages (except for typos), even if I think the message was not necessary. All I did was to change the position of the message as a parameter, since this changed from JUnit 4 to 5. My intention was to minimize the changes, and afterall, some of the messages give information about the failure of the test that the message from JUnit will not give.
I'll try to keep only meaningful messages and remove the redundant ones, that only states the test (like saying "xyz is not true" for a assertTrue test).
There was a problem hiding this comment.
My comment was not about assertion messages, but about the extra {} that are superfluous.
There was a problem hiding this comment.
Yes, sorry not letting this clear. I understood and will remove them. I only commented about the messages because when I started the rebase process (as instructed by @aherbert), I noticed that some messages were removed.
| } catch (final IndexOutOfBoundsException e) { | ||
| // Expected | ||
| } | ||
| assertThrows(IndexOutOfBoundsException.class, () -> { |
There was a problem hiding this comment.
@itamarc
Same comment as for the other extra {} in other files...
TY!
9cc5f44 to
2ac6475
Compare
unit-vintage-engine dropped from pom.xml
11e66ac to
9825aef
Compare
|
I made the rebase with the origin/master and looks like I solved all conflicts and I just squashed the commits into 1. I also removed the extra curly braces like pointed by @garydgregory If there are something else I need to do, please point out. |
daniel-tavares-osf
left a comment
There was a problem hiding this comment.
Very nice work. Congratulations.
unit-vintage-engine dropped from pom.xml
I migrated all tests in commons-coded to use JUnit5.
I also dropped junit-vintage-engine from pom.xml, since it's not needed anymore.