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

Make Localization.relativeTime testable #5358

Merged
merged 2 commits into from
Jan 13, 2021

Conversation

XiangRongLin
Copy link
Collaborator

@XiangRongLin XiangRongLin commented Jan 6, 2021

What is it?

  • Codebase improvement (dev facing)

Description of the changes in your PR

  • Make Localization.relativeTime testable.
  • Add some simple tests

Context

APK testing

https://github.com/TeamNewPipe/NewPipe/actions/runs/466273618

Due diligence

Problem is global state in static variable prettyTime. But for performance reasons on Android that is preferred.
Now allow injecting prettyTime dependency by making init function public.

val actual = Localization.relativeTime(GregorianCalendar(2021, 1, 6))

assertEquals("1 month from now", actual)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am just unable to understand Kotlin, in this case please enlighten me. But this assertion will never be true because both times are just 5 days apart from each other - especially when I compare it with the last test? So shouldn't it be assertEquals("5 days from now", actual) here as well?

Copy link
Collaborator Author

@XiangRongLin XiangRongLin Jan 13, 2021

Choose a reason for hiding this comment

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

You are absolutly correct, but this is actually what pretty time returns. I would guess it's either a bug in PrettyTime or a limitation when comparing with GregorianCalendar.

For these tests i went with the assumption that the current state is correct. So i just added assertions with what is returned, so future regressions are covered.

Copy link
Member

Choose a reason for hiding this comment

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

okay, can you add this explanation as an inline comment so that no one stumbles upon this later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean the first paragraph of the second (or both)?

I would add the first with the note that further research is required

Copy link
Member

Choose a reason for hiding this comment

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

just add anything meaningful like // yes this assertion is true, even if it should be 5 days, it works as it is. Future research required

Maybe it has to do something with timezone shift. Could it be that Locale.ENGLISH is defaulting to the global standard locale US in Android? This would explain why 2021, 1, 1 is in fact 2020,12,12 normalized to UTC and could explain "1 month from now"

@Redirion Redirion merged commit 0264383 into TeamNewPipe:dev Jan 13, 2021
@Redirion
Copy link
Member

sigh did just see #5230

mayb we should have merged this earlier

@XiangRongLin XiangRongLin deleted the testable_prettytime branch January 14, 2021 03:59
@XiangRongLin XiangRongLin mentioned this pull request Jan 14, 2021
5 tasks
This was referenced Jan 18, 2021
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 20, 2021
…e_prettytime

Make Localization.relativeTime testable
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.

2 participants