Move Android test as jvm test: TabDataRepository, and fix flaky test #5316
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| @@ -468,7 +485,7 @@ class TabDataRepositoryTest { | |||
| @Test | |||
| fun getActiveTabCountReturnsCorrectCountWhenTabsYoungerThanSpecifiedDay() = runTest { | |||
There was a problem hiding this comment.
@0nko after fixing the flaky test, this one is now failing. Can you evaluate the expected result?
There was a problem hiding this comment.
The fake DateTime provider revealed the timing issue, which made it pass even though it shouldn’t, nice! 👍 It’s not this test that’s failing, it’s getInactiveTabCountReturnsCorrectCountWhenAllTabsOlderThanSpecifiedDay() below (see my comment).
| @@ -199,7 +199,7 @@ class TabDataRepository @Inject constructor( | |||
| } | |||
|
|
|||
| override fun countTabsAccessedWithinRange(accessOlderThan: Long, accessNotMoreThan: Long?): Int { | |||
| val now = LocalDateTime.now(ZoneOffset.UTC) | |||
| val now = timeProvider.localDateTimeNow() | |||
There was a problem hiding this comment.
@0nko please review this change, moving to use an injected TimeProvider.
There was a problem hiding this comment.
That’s fine, but to save the last access time, we specify the UTC timezone by default. We should update it there, as well, to be consistent.
There was a problem hiding this comment.
Thank you @cmonfortep! I left a couple of comments in the code to fix the failing test and one more thing.
| @@ -199,7 +199,7 @@ class TabDataRepository @Inject constructor( | |||
| } | |||
|
|
|||
| override fun countTabsAccessedWithinRange(accessOlderThan: Long, accessNotMoreThan: Long?): Int { | |||
| val now = LocalDateTime.now(ZoneOffset.UTC) | |||
| val now = timeProvider.localDateTimeNow() | |||
There was a problem hiding this comment.
That’s fine, but to save the last access time, we specify the UTC timezone by default. We should update it there, as well, to be consistent.
| @@ -468,7 +485,7 @@ class TabDataRepositoryTest { | |||
| @Test | |||
| fun getActiveTabCountReturnsCorrectCountWhenTabsYoungerThanSpecifiedDay() = runTest { | |||
There was a problem hiding this comment.
The fake DateTime provider revealed the timing issue, which made it pass even though it shouldn’t, nice! 👍 It’s not this test that’s failing, it’s getInactiveTabCountReturnsCorrectCountWhenAllTabsOlderThanSpecifiedDay() below (see my comment).
| @@ -497,7 +514,7 @@ class TabDataRepositoryTest { | |||
| @Test | |||
| fun getInactiveTabCountReturnsCorrectCountWhenAllTabsOlderThanSpecifiedDay() = runTest { | |||
| // Arrange: Add some tabs with different last access times | |||
| val now = LocalDateTime.now(ZoneOffset.UTC) | |||
| val now = now() | |||
| val tab1 = TabEntity(tabId = "tab1", lastAccessTime = now.minusDays(8)) | |||
| val tab2 = TabEntity(tabId = "tab2", lastAccessTime = now.minusDays(10)) | |||
| val tab3 = TabEntity(tabId = "tab3", lastAccessTime = now.minusDays(9)) | |||
There was a problem hiding this comment.
The fix
| val tab3 = TabEntity(tabId = "tab3", lastAccessTime = now.minusDays(9)) | |
| val tab3 = TabEntity(tabId = "tab3", lastAccessTime = now.minusDays(9).minusSeconds(1)) |
Task/Issue URL: https://app.asana.com/0/1202552961248957/1208839731155341/f
Description
Fixes flaky test due time reference.
Moves it as jvm.
Steps to test this PR
Feature 1
UI changes