-
Notifications
You must be signed in to change notification settings - Fork 60
Improvements to test.ceylon.time #570
Improvements to test.ceylon.time #570
Conversation
Can one of the admins verify this patch? (see README) |
@CeylonBuildBot ok to test |
@@ -141,7 +141,7 @@ shared test void testDateMinusMonths() { | |||
} | |||
|
|||
shared test void testDateMinusLessDays() { | |||
assertEquals( date(1982,november,30), date(1982,december,31).minusMonths(1)); | |||
assertEquals( date(1982,december,31).minusMonths(1), date(1982,november,30) ); |
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.
These could also be converted to named argument invocation style.
I find I've come to like it much more than ordinal invocation. Specially for test assertions.
this particualtr one would look much better imho if it was formatted like this:
shared test void testDateMinusLessDays() => assertEquals {
actual = date(1982,december,31).minusMonths(1);
expected = date(1982,november,30);
};
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.
What about the test methods with more than one assertEquals
? That would be a lot more lines…
Wow @lucaswerkmeister you must really have a ton of unaccounted for time on your hands :) Thanks, this is much better now 👍 |
Should I merge it or do you want to make these last few adjustments...? |
I can make the adjustments. Should I amend the existing commit or add another one? |
Oh, I forgot I wanted to make another change as well – rename members like |
probably thats a typo @lucaswerkmeister .. in Brazil date is data .. probably I wrote in portuguese by mistake |
Alright, all done except for the non-named arguments Also, the question of commits is still open… 7d2d111 should at least be squashed into c68b01e, but apart from that, I leave it to you if you want to merge the separate commits or a squashed commit. (I think squashing would make more sense.) |
I guess, just adding commits to this PR is good enough... but if you like, you can squash tem .. the resulting history does look nicer when merged :) (I personally have only seldom bothered to though) |
- Many, MANY tests were calling assertEquals with the wrong argument order: assertEquals(expected, actual), whereas the correct order with positional arguments is assertEquals(actual, expected). A few of these mistakes were caught by eclipse-archived/ceylon#6183. I attempted to fix them in a way that would be consistent with other tests in the file, so sometimes I swapped the parameters, and sometimes I changed the invocation to use named arguments. - Some tests did manual exception handling instead of using assertThatException. - Some tests could be parameterized (eclipse-archived#159). - Some date_* variables were misspelled as data_*.
0aa7a39
to
dc45106
Compare
Alright, I’ve pushed a squashed commit. (Also rebased on current |
Thanks Lucas! |
You’re welcome! :) |
assertEquals
with the wrong argument order:assertEquals(expected, actual)
, whereas the correct order with positional arguments isassertEquals(actual, expected)
. A few of these mistakes were caught by Implement #6172: parameter/argument warning ceylon#6183. I attempted to fix them in a way that would be consistent with other tests in the file, so sometimes I swapped the parameters, and sometimes I changed the invocation to use named arguments.assertThatException
.