Date factory test when date object is passed #15998
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsteur
commented
May 27, 2020
| @@ -60,7 +60,7 @@ public function trackVisits() | |||
| $this->trackEventWithoutUrl($vis); | |||
| $this->trackMovieWatchingIncludingInterval($vis); | |||
|
|
|||
| $this->dateTime = Date::factory($this->dateTime)->addHour(0.5); | |||
| $this->dateTime = Date::factory($this->dateTime)->addHour(0.5)->getDatetime(); | |||
| $vis2 = self::getTracker($this->idSite, $this->dateTime, $useDefault = true, $uselocal); | |||
There was a problem hiding this comment.
we used to pass a Date object here meaning the tracker would do $date->toString() which then would only result in YYYY-MM-DD and throw away the entire time. So this was before actually not working which is why some tests are failing.
tsteur
commented
May 27, 2020
| @@ -47,7 +47,7 @@ public function trackVisits() | |||
|
|
|||
| $this->trackContentImpressionsAndInteractions($vis); | |||
|
|
|||
| $this->dateTime = Date::factory($this->dateTime)->addHour(0.5); | |||
| $this->dateTime = Date::factory($this->dateTime)->addHour(0.5)->getDatetime(); | |||
There was a problem hiding this comment.
we used to pass a Date object here meaning the tracker would do $date->toString() which then would only result in YYYY-MM-DD and throw away the entire time. So this was before actually not working which is why some tests are failing.
tsteur
added
the
Pull Request WIP
tsteur
added
Needs Review
|
Any thoughts @sgiehl @diosmosis ? |
|
In general I would vote for fixing the tests and merging this change. That is actually the behavior I imho would have expected before 🙈 |
|
👍 fixed the tests @sgiehl |
mattab
added
not-in-changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As suggested in chat made this change to see if anything breaks.
There are some failing tests in CustomEvents and Content tracking but that's because of how the date was falsely used in the test fixture. Happy to update the tests and add a changelog entry if we are keen to merge this. Not sure what else it would maybe break that maybe isn't covered by tests.