Skip to content

Add test for bug #52480 #2539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add test for bug #52480 #2539

wants to merge 2 commits into from

Conversation

michaelmoussa
Copy link
Contributor

The bug seems to manifest depending on the timezone, so this test iterates through all available timezones to verify each calculated interval is correct.

https://bugs.php.net/bug.php?id=52480

@krakjoe
Copy link
Member

krakjoe commented May 29, 2017

The expect section does not look correct.

@michaelmoussa
Copy link
Contributor Author

@krakjoe if the date math works correctly, then the expected output is a long string of 1s.

A series of strings with the timezone and result so it's more explicit might be better, i.e. UTC:1\n and so on, but the objective is to test that the math works out, not that there's any specific number or ordering of timezones.

Do you have a better suggestion/preference? I'm happy to modify it if so.

@krakjoe
Copy link
Member

krakjoe commented Jun 1, 2017

Sorry, I misread the diff (didn't notice +) ...

The test fails, but I guess that's expected ... if it's expected can you add an xfail section please ?

@nikic
Copy link
Member

nikic commented Jun 2, 2017

What we usually do for tests like this is not output anything in the success case and have a

?>
===DONE===
--EXPECT--
===DONE===

as the expectation.

@nikic
Copy link
Member

nikic commented Jun 3, 2017

@weltling Can you please check the AppVeyor build? The test is failing and it says NMAKE : fatal error U1077: 'C:\obj\Release\php.exe' : return code '0x1', but the whole build passes.

@michaelmoussa
Copy link
Contributor Author

@krakjoe, I added XFAIL and ===DONE=== expectation for success case per @nikic's suggestion.

@weltling
Copy link
Contributor

weltling commented Jun 5, 2017

@nikic yeah, that's fishy, looking after it now. Seems something changed at least with master, not yet clear to me where.

Thanks.

@krakjoe
Copy link
Member

krakjoe commented Jun 22, 2017

Merged 9437dcd

Thanks.

@krakjoe krakjoe closed this Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants