Skip to content
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

Add timezone function tests #553

Closed

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Nov 15, 2021

This adds test cases for addTimezoneGuesser and addTimezoneFinder on top of #548

I just tests that the functions can be called without throwing an exception! That at least verifies that the functions still exist and can be called with the tested parameters.

There is not getTimezoneGuessers() function, so it is not immediately easy to know if the add function did anything. To fully test we would have to provide some new-and-different guesser(s), add it, then do some calls to findTimeZone and observe a difference in behavior. I'm not sure how much of that we should try and do.

But I am also not sure if these "mindless" test scenarios are worth having.

heiglandreas and others added 2 commits November 15, 2021 14:50
This will ease customization of timezone-guessing as it is now gets easier
to extend that process with own implementations (as long as they
implement the appropriate interface)

This is espechially necessary when wanting to actually guess a timezone
via the rules defined in the VTIMEZONE-entry (which is currently not
done)
@phil-davis phil-davis self-assigned this Nov 15, 2021
@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #553 (806643a) into master (a7460c5) will decrease coverage by 0.22%.
The diff coverage is 99.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #553      +/-   ##
============================================
- Coverage     98.69%   98.47%   -0.23%     
- Complexity     1818     1848      +30     
============================================
  Files            66       71       +5     
  Lines          4454     4521      +67     
============================================
+ Hits           4396     4452      +56     
- Misses           58       69      +11     
Impacted Files Coverage Δ
lib/TimezoneGuesser/FindFromTimezoneIdentifier.php 92.85% <92.85%> (ø)
lib/TimeZoneUtil.php 80.35% <100.00%> (-17.38%) ⬇️
lib/TimezoneGuesser/FindFromOffset.php 100.00% <100.00%> (ø)
lib/TimezoneGuesser/FindFromTimezoneMap.php 100.00% <100.00%> (ø)
lib/TimezoneGuesser/GuessFromLicEntry.php 100.00% <100.00%> (ø)
lib/TimezoneGuesser/GuessFromMsTzId.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7460c5...806643a. Read the comment docs.

@phil-davis
Copy link
Contributor Author

@staabm @heiglandreas
should I add the last 2 commits here to PR #548 ?
It covers a few more little things that codecov was complaining about.
But maybe the "tests" that I wrote for the add timezone guesser functions are rather mindless/useless?

Opinion please!

@staabm
Copy link
Member

staabm commented Nov 15, 2021

hmm addFinder and addGuesser are simple adder/setter methods which cannot throw.
I don't see where/when a exception could/would be thrown?

@phil-davis
Copy link
Contributor Author

phil-davis commented Nov 15, 2021

hmm addFinder and addGuesser are simple adder/setter methods which cannot throw. I don't see where/when a exception could/would be thrown?

That's the challenge of these simple functions! If someone changes it some day and breaks it (in a way that throws an exception, or they rename or delete the function) then the test will fail. So it protects against obvious "regressions" like that.

I'm not sure how to really do better than that, without creating some new example guesser and using it...

@staabm
Copy link
Member

staabm commented Nov 15, 2021

I would tackle this problem not on a per method basis, but with a BC check which covers the whole codebase/project regarding BC, e.g. https://github.com/Roave/BackwardCompatibilityCheck

@phil-davis
Copy link
Contributor Author

I added testEmptyTimeZone to PR #548 which is now merged.

The other tests are not that useful.

@phil-davis phil-davis closed this Nov 16, 2021
@phil-davis phil-davis deleted the addTimezone-function-tests branch November 16, 2021 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants