Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Improvements to test.ceylon.time #570

Merged
merged 1 commit into from
May 9, 2016

Conversation

lucaswerkmeister
Copy link

@lucaswerkmeister lucaswerkmeister commented May 5, 2016

  • 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 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.
  • Some tests did manual exception handling instead of using assertThatException.
  • Some tests could be parameterized (ceylon.test: support parametrized tests #159).

@CeylonBuildBot
Copy link

Can one of the admins verify this patch? (see README)

@lucaswerkmeister
Copy link
Author

CC @DiegoCoronel, @luolong

@tombentley
Copy link

@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) );
Copy link
Contributor

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);
};

Copy link
Author

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…

@luolong
Copy link
Contributor

luolong commented May 6, 2016

Wow @lucaswerkmeister you must really have a ton of unaccounted for time on your hands :)

Thanks, this is much better now 👍

@luolong
Copy link
Contributor

luolong commented May 6, 2016

Should I merge it or do you want to make these last few adjustments...?

@lucaswerkmeister
Copy link
Author

I can make the adjustments. Should I amend the existing commit or add another one?

@lucaswerkmeister
Copy link
Author

Oh, I forgot I wanted to make another change as well – rename members like data_1982_12_13 to date_1982_12_13. Or is that not a typo?

@DiegoCoronel
Copy link
Contributor

probably thats a typo @lucaswerkmeister .. in Brazil date is data .. probably I wrote in portuguese by mistake

@lucaswerkmeister
Copy link
Author

Alright, all done except for the non-named arguments assertEquals in testDates.ceylon. Should I change them all to 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.)

@luolong
Copy link
Contributor

luolong commented May 9, 2016

@lucaswerkmeister:

Should I amend the existing commit or add another one?

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_*.
@lucaswerkmeister
Copy link
Author

lucaswerkmeister commented May 9, 2016

Alright, I’ve pushed a squashed commit. (Also rebased on current master.)

@luolong luolong merged commit dd9b2f5 into eclipse-archived:master May 9, 2016
@luolong
Copy link
Contributor

luolong commented May 9, 2016

Thanks Lucas!

@lucaswerkmeister lucaswerkmeister deleted the testCeylonTime branch May 9, 2016 18:05
@lucaswerkmeister
Copy link
Author

You’re welcome! :)

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

Successfully merging this pull request may close these issues.

5 participants