Test enhancement #2

peter279k · 2018-03-04T02:37:21Z

Changed log

add more tests for other uncovered classes.

BenMorel · 2018-03-05T12:43:33Z

tests/InstantTest.php

+     */
+    public function testIsBetweenExclusive(int $s1, int $n1, int $s2, int $n2, int $cmp)
+    {
+        $this->assertSame($cmp <= 0, Instant::of($s1, $n1)->isBetweenExclusive(Instant::of($s2, $n2), Instant::of($s2, $n2)));


You're not testing much in these two methods, you're only testing that the value is within an interval of 0 seconds.

What I would do here is hardcode the interval start and end, and test with a range of possible values in between:

public function testIsBetweenExclusive(int $seconds, int $nanos, bool $isBetween) { $this->assertSame($isBetween, Instant::of($seconds, $nanos)->isBetweenExclusive( Instant::of(-1, -1), Instant::of(1, 1) ); }

And for the provider:

[ [-1, -2, false], [-1, -1, false], [-1, 0, true], [-1, 1, true], [0, -1, true], [0, 0, true], [0, 1, true], [1, -1, true], [1, 0, true], [1, 1, false], [1, 2, false], ]

Same for isBetweenInclusive(), with the data provider altered accordingly.

BenMorel · 2018-03-06T12:44:19Z

tests/WeekOfYearTest.php

+     * @expectedException        Brick\DateTime\DateTimeException
+     * @expectedExceptionMessage Year 2000 does not have 53 weeks
+     */
+    public function testCehckTimeShouldThrowDateYimeException()


There are still 2 typos: 'Cehck' instead of 'Check' and 'Yime' instead of 'Time'!

BenMorel · 2018-03-11T13:13:59Z

tests/InstantTest.php

+            [ 1,  0,  0,  1,  1],
+        ];
+    }
+


These providers are obsolete!

I will remove this data provider.

BenMorel · 2018-03-11T13:19:48Z

tests/WeekOfYearTest.php

+     * @expectedException        Brick\DateTime\DateTimeException
+     * @expectedExceptionMessage Invalid week-of-year: -1 is not in the range 1 to 53.
+     */
+    public function testCheckShouldReturnDateTimeExceptionWithFieldNotInRange()


Return => Throw

BenMorel · 2018-03-11T13:20:06Z

tests/PatternParserBuilderTest.php

+     * @expectedException        \RuntimeException
+     * @expectedExceptionMessage Cannot call endOptional() without a call to startOptional() first.
+     */
+    public function testEndOptionalShouldReturnRuntimeException()


Return => Throw

BenMorel · 2018-03-11T13:20:14Z

tests/PatternParserBuilderTest.php

+     * @expectedException        \RuntimeException
+     * @expectedExceptionMessage Cannot call endGroup() without a call to startGroup() first.
+     */
+    public function testEndGroupShouldReturnRunTimeException()


Return => Throw

BenMorel · 2018-03-11T13:22:44Z

tests/YearMonthTest.php

+    public function testWithYearIsTheSameYear()
+    {
+        $year = (int)date('Y');
+        $month = (int)date('m');


Please use fixed year/month here

BenMorel · 2018-03-11T13:25:29Z

tests/YearMonthTest.php

+        $month = (int)date('m');
+
+        $this->assertInstanceOf(YearMonth::class, YearMonth::of($year, $month)->withYear($year));
+        $this->assertYearMonthIs($year, $month, YearMonth::of($year, $month)->withYear($year));


You should instantiate YearMonth::of(...) only once and use it in 2 lines.

BenMorel · 2018-03-11T13:25:53Z

tests/YearMonthTest.php

    public function testWithMonth()
    {
        $this->assertYearMonthIs(2000, 12, YearMonth::of(2000, 1)->withMonth(12));
    }

+    public function testWithMonthWithSameMonth()
+    {
+        $month = (int)date('m');


Please use a fixed month here

BenMorel · 2018-03-11T13:26:46Z

tests/YearMonthTest.php

@@ -386,6 +407,12 @@ public function testPlusYears(int $year, int $month, int $plusYears, int $expect
        $this->assertYearMonthIs($expectedYear, $expectedMonth, $yearMonth->plusYears($plusYears));
    }

+    public function testPlusYearsWithParameterIsZero()


testPlusZeroYears()?

I agree that and it seems that the test method name can be short.

BenMorel · 2018-03-11T13:35:24Z

tests/YearWeekTest.php

+    public function testNow()
+    {
+        $timeZone = TimeZone::parse('Asia/Taipei');
+        $yearWeek = YearWeek::now($timeZone);


You should provide a Clock here, and check that the resulting YearWeek is in line with the clock!

Can you give some examples about this?

Thanks.

You can set set timestamp that now() will use, this way:

$clock = new FixedClock(Instant::of(2000000000)); $yearWeek = YearWeek::now($timeZone, $clock);

BenMorel · 2018-03-11T13:40:49Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame('Thursday', (string)$zonedDateTime->plusPeriod(Period::ofWeeks(11))->getDayOfWeek());


Please assert the full string here: assertSame('2000-...', (string) $zonedDateTime->plusPeriod(Period::ofWeeks(11)));

BenMorel · 2018-03-11T13:40:58Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(56, $zonedDateTime->plusDuration(Duration::zero())->getSecond());


Same as above

BenMorel · 2018-03-11T13:41:08Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(2002, $zonedDateTime->plusYears(2)->getYear());


Same as above

BenMorel · 2018-03-11T13:41:14Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(3, $zonedDateTime->plusMonths(2)->getMonth());


Same as above

BenMorel · 2018-03-11T13:41:23Z

tests/ZonedDateTimeTest.php

+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(2, $zonedDateTime->plusWeeks(2)->getMonth());
+        $this->assertSame(3, $zonedDateTime->plusWeeks(2)->getDay());


Same as above

BenMorel · 2018-03-11T13:41:30Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(22, $zonedDateTime->plusDays(2)->getDay());


Same as above

BenMorel · 2018-03-11T13:41:38Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(14, $zonedDateTime->plusHours(2)->getHour());


Same as above

BenMorel · 2018-03-11T13:41:48Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(36, $zonedDateTime->plusMinutes(2)->getMinute());


Same as above

BenMorel · 2018-03-11T13:41:54Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(58, $zonedDateTime->plusSeconds(2)->getSecond());


Same as above

BenMorel · 2018-03-11T13:41:59Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame('Thursday', (string)$zonedDateTime->minusPeriod(Period::ofWeeks(11))->getDayOfWeek());


Same as above

BenMorel · 2018-03-11T13:42:03Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(56, $zonedDateTime->minusDuration(Duration::zero())->getSecond());


Same as above

BenMorel · 2018-03-11T13:42:09Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(1999, $zonedDateTime->minusYears(1)->getYear());


Same as above

BenMorel · 2018-03-11T13:42:16Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(12, $zonedDateTime->minusMonths(1)->getMonth());


Same as above

BenMorel · 2018-03-11T13:42:20Z

tests/ZonedDateTimeTest.php

+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame(1, $zonedDateTime->minusWeeks(2)->getMonth());
+        $this->assertSame(6, $zonedDateTime->minusWeeks(2)->getDay());


Same as above

BenMorel · 2018-03-12T17:10:28Z

tests/Field/DayOfMonthTest.php

+    {
+        $dayOfMonth = new DayOfMonth();
+
+        $this->assertNull($dayOfMonth->check(31));


I'm not comfortable with assertNull() for a void method. To remove the assertion and avoid a "risky" test, maybe you can give a try to @doesNotPerformAssertions?

It sounds good because it's the void method and not return the null currently.

BenMorel · 2018-03-12T21:36:10Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame('2000-01-10T12:34:56.123456789-08:00', (string) $zonedDateTime->withFixedOffsetTimeZone(56));


withFixedOffsetTimeZone() does not take any parameters!

BenMorel · 2018-03-12T21:36:21Z

tests/ZonedDateTimeTest.php

+
+    public function testPlusPeriod()
+    {
+


BenMorel · 2018-03-12T21:37:16Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame('2000-01-20T12:34:56.123456789-08:00[America/Los_Angeles]', (string) $zonedDateTime->plusDuration(Duration::zero()));


Could you please use another Duration than zero?

BenMorel · 2018-03-12T21:38:00Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame('2000-01-20T12:34:56.123456789-08:00[America/Los_Angeles]', (string) $zonedDateTime->minusDuration(Duration::zero()));


Same, a non-zero Duration would be more useful!

BenMorel · 2018-03-12T21:40:47Z

tests/DateTimeParseResultTest.php

+
+declare(strict_types=1);
+
+namespace Brick\DateTime\Tests;


This should be in namespace Brick\DateTime\Tests\Parser, and located in a tests/Parser directory

BenMorel · 2018-03-12T21:51:21Z

tests/PatternParserBuilderTest.php

+
+declare(strict_types=1);
+
+namespace Brick\DateTime\Tests;


This should be in namespace Brick\DateTime\Tests\Parser, and located in a tests/Parser directory

BenMorel · 2018-03-12T21:51:46Z

tests/PatternParserBuilderTest.php

+use Brick\DateTime\Parser\PatternParserBuilder;
+
+/**
+ * Unit tests for class IsoParsers.


for class PatternParserBuilder.

BenMorel · 2018-03-12T21:52:31Z

tests/DateTimeParseResultTest.php

+class DateTimeParseResultTest extends AbstractTestCase
+{
+    /**
+     * @expectedException        Brick\DateTime\Parser\DateTimeParseException


Please add leading \ before exception class

BenMorel · 2018-03-12T21:52:42Z

tests/Field/WeekOfYearTest.php

+class WeekOfYearTest extends AbstractTestCase
+{
+    /**
+     * @expectedException        Brick\DateTime\DateTimeException


Please add leading \ before exception class

BenMorel · 2018-03-12T21:52:48Z

tests/Field/WeekOfYearTest.php

+    }
+
+    /**
+     * @expectedException        Brick\DateTime\DateTimeException


Please add leading \ before exception class

BenMorel · 2018-03-12T21:52:55Z

tests/IntervalTest.php

@@ -12,6 +12,18 @@
 */
 class IntervalTest extends AbstractTestCase
 {
+    /**
+     * @expectedException        Brick\DateTime\DateTimeException


Please add leading \ before exception class

BenMorel · 2018-03-12T21:53:29Z

tests/PatternParserBuilderTest.php

+     * @expectedException        \RuntimeException
+     * @expectedExceptionMessage Cannot call endGroup() without a call to startGroup() first.
+     */
+    public function testEndGroupShouldThrowRunTimeException()


RunTime => Runtime

BenMorel · 2018-03-12T21:53:50Z

tests/YearMonthTest.php

@@ -330,16 +330,38 @@ public function testWithYear()
        $this->assertYearMonthIs(2001, 5, YearMonth::of(2000, 5)->withYear(2001));
    }

+    public function testWithYearIsTheSameYear()


Please rename to testWithYearWithSameYear

BenMorel · 2018-03-12T21:54:39Z

tests/YearMonthTest.php

+        $month = 2;
+        $yearMonth = YearMonth::of($year, $month);
+
+        $this->assertInstanceOf(YearMonth::class, $yearMonth->withYear($year));


This line is not needed, assertYearMonthIs() will throw a TypeError if it's not an instance of YearMonth.

BenMorel · 2018-03-12T21:55:27Z

tests/YearMonthTest.php

    public function testGetFirstDay()
    {
        $this->assertLocalDateIs(2023, 10, 1, YearMonth::of(2023, 10)->getFirstDay());
    }

+    public function testGetLastDay()


Actually there was already a test for getLastDay(), that was not executed because it was wrongly named!
You can remove this method, and rename my test getGetLastDay to testGetLastDay!

BenMorel · 2018-03-12T22:01:50Z

tests/YearWeekTest.php

+        $yearWeek = YearWeek::now($timeZone, $now);
+
+        $this->assertSame('2033-W20', (string) $yearWeek);
+        $this->assertInstanceOf(YearWeek::class, $yearWeek);


Can you use assertYearWeekIs() instead of these 2 lines?

BenMorel · 2018-03-12T22:03:00Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertDayOfWeekIs(4, $zonedDateTime->getDayOfWeek());


Please use the DayOfWeek::THURSDAY constant instead of 4 for readability

BenMorel · 2018-03-12T22:04:13Z

tests/ZonedDateTimeTest.php

+        $localDateTime = LocalDateTime::parse($localDateTime);
+        $zonedDateTime = ZonedDateTime::of($localDateTime, $timeZone);
+
+        $this->assertSame('2000-03-25T12:34:56.123456789-08:00[America/Los_Angeles]', (string) $zonedDateTime->withSecond(56));


Can you use another second value in withSecond()? It's the same as in the original date-time!

BenMorel · 2018-03-12T22:07:20Z

tests/Field/DayOfMonthTest.php

+/**
+ * Unit tests for class DayOfMonth.
+ *
+ * @doesNotPerformAssertions


Does it work with the annotation on the method rather than on the class?
It would be better if we add other methods in the future.

Yes. I set the wrong place for the annotation.

BenMorel · 2018-03-12T22:09:11Z

tests/Field/DayOfMonthTest.php

+    {
+        $dayOfMonth = new DayOfMonth();
+
+        $dayOfMonth->check(31);


check() is a static method, it should be DayOfMonth::check(31); and the first line can be removed.

BenMorel · 2018-03-13T16:44:53Z

I think you forgot to reintroduce DateTimeParseResultTest and PatternParserBuilderTest, they're missing!

BenMorel · 2018-03-14T10:22:48Z

Looks good now 👍 Thanks!

peter279k added 2 commits March 4, 2018 09:44

test enhancement

5c0c5df

add parser throws exception test

5b03631

BenMorel reviewed Mar 5, 2018

View reviewed changes

peter279k added 2 commits March 6, 2018 00:25

add other date tests and approve request changes

d9b4438

fix typo test method name, orgnize data provider

692edce

BenMorel reviewed Mar 6, 2018

View reviewed changes

fix "check", "time" typo

ff264cd

BenMorel reviewed Mar 11, 2018

View reviewed changes

using past and future fixed clock data provider

adfca83

BenMorel reviewed Mar 12, 2018

View reviewed changes

add @doesNotPerformAssertions for this test

a6c8907

BenMorel reviewed Mar 12, 2018

View reviewed changes

tests/ZonedDateTimeTest.php Outdated

public function testPlusPeriod()

{

Copy link

Member

BenMorel Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra line

BenMorel reviewed Mar 12, 2018

View reviewed changes

peter279k added 2 commits March 13, 2018 23:01

approve the request changes

760d417

fix DayOfMonth variable

c3cddc3

move the file to tests/Parser

f44349b

BenMorel merged commit a878173 into brick:master Mar 14, 2018

peter279k deleted the test_enhancement branch August 25, 2020 10:31

Test enhancement #2

Test enhancement #2

Conversation

peter279k commented Mar 4, 2018

Changed log

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter279k Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter279k Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMorel Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMorel commented Mar 13, 2018

BenMorel commented Mar 14, 2018

peter279k Mar 11, 2018 •

edited

Loading

peter279k Mar 11, 2018 •

edited

Loading

BenMorel Mar 12, 2018 •

edited

Loading