e107inc / e107-test

For developmental use with e107 v2.
2 stars 2 forks source link

remaining e_date tests #4

Closed SimSync closed 6 years ago

Deltik commented 6 years ago

Some of these new tests have failed:

There were 2 failures:

---------
1) e_dateTest: Strptime
 Test  tests/unit/e_dateTest.php:testStrptime
Failed asserting that two arrays are equal.
- Expected | + Actual
@@ @@
'tm_fmon' => 'May'
'tm_wday' => 0
'tm_yday' => 132
+    'tm_sec' => 0
+    'tm_min' => 0
+    'tm_hour' => 0
+    'unparsed' => ''
+    'tm_amon' => 'May'
)
#1  /home/deltikdev/repos/e107-test/tests/unit/e_dateTest.php:164

---------
2) e_dateTest: Convert_date
 Test  tests/unit/e_dateTest.php:testConvert_date
Failed asserting that two strings are equal.
- Expected | + Actual
@@ @@
-'12:45 PM'
+'12:45 pm'
#1  /home/deltikdev/repos/e107-test/tests/unit/e_dateTest.php:190

FAILURES!
Tests: 36, Assertions: 105, Failures: 2.

Is there an e107 commit or pull request that I need to make these tests pass?

SimSync commented 6 years ago

I've changed something for strptime on windows. But the Linux part is unchanged. This change is included in the official repo. regarding the second one, this might be a typo from me. I have to check this.

Deltik commented 6 years ago

The outputs should be the same regardless of platform and supported PHP version. I'm concerned that e107 will behave differently on Windows and Unix.

Are you working on porting e_date::strptime() to Windows so that the result is identical to other platforms?

SimSync commented 6 years ago

The issue came up when I wrote a test for toTime(). The result is wrong on windows. And the test fails on windows. That it didn't fail for you only supports my opinion that the strptime function is buggy. I think, we should create a new function based on https://secure.php.net/manual/en/function.date-parse-from-format.php date_parse_from_format(), which is available on all platforms and has not the issues which strptime has.

Deltik commented 6 years ago

How many usages of e_date::strptime() would we have to account for to make a cross-platform alternative?

The only usage I see in the core is that e_date::toTime() function that you mentioned.

CaMer0n commented 6 years ago

@Deltik I made changes to toTime() so it uses date_parse_from_format(), Still needs more testing.. and maybe the test improved for it?

Deltik commented 6 years ago

@CaMer0n: Well, the tests that are failing for me specifically target the e_date::strptime() method, so there has not been a change in the test output.

e_dateTest::testToTime() is the one test that specifically tests e_date::toTime(), and it's still passing, so that's promising.

SimSync commented 6 years ago

@CaMer0n For me the tests now succeed (toTime and strptime). @Deltik What is the output when you run the strptime tests? What is the value of the actual array?

Deltik commented 6 years ago

@SimSync: Unchanged from my first comment

SimSync commented 6 years ago

@Deltik So, your $actual array has additional entries, that are missing in the $expected array (tm_sec, tm_min, tm_hour, unparsed), but the existing entries match the expected ones (maybe different order...). OK, i'll check the strptime function for the missing entries.

CaMer0n commented 6 years ago

strptime is not used by the core, but I guess there is always a chance that a third-party plugin is using it.