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.

Sign up for GitHub

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

[Bug] Fix root cause that caused StatsGranularTabsTest to fail. #20606

Merged
merged 9 commits into from
Apr 17, 2024
Merged

[Bug] Fix root cause that caused StatsGranularTabsTest to fail. #20606

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b20ce69
[Bug] Fixed bug where stats card doesn't always show up on clean inst…
Apr 5, 2024
619252f
[Test] Uncomment previously failing UI test.
Apr 6, 2024
9275240
Merge branch 'trunk' into andy/wp-android-issue-18065
Apr 9, 2024
1fd534f
[Bug] Updated the state check and removed uneccessary cache load.
Apr 9, 2024
927303a
[Bug] Remove an if level.
Apr 9, 2024
21c1721
Update unit test that I noticed were failing.
Apr 11, 2024
32754de
Rename some tests and removed test lib I accidentally added.
Apr 12, 2024
1ecc095
Merge branch 'trunk' into andy/wp-android-issue-18065
Apr 16, 2024
71d7ad3
Make further review updates.
Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions WordPress/src/androidTest/java/org/wordpress/android/e2e/StatsGranularTabsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@ import dagger.hilt.android.testing.HiltAndroidTest
import org.junit.After
import org.junit.Assume.assumeTrue
import org.junit.Before
import org.junit.Ignore
import org.junit.Test
import org.wordpress.android.BuildConfig
import org.wordpress.android.R
import org.wordpress.android.e2e.pages.MySitesPage
import org.wordpress.android.rules.Retry
import org.wordpress.android.support.BaseTest
import org.wordpress.android.support.ComposeEspressoLink
import org.wordpress.android.support.WPSupportUtils
Expand All @@ -38,7 +36,6 @@ class StatsGranularTabsTest : BaseTest() {
}
}

@Ignore("The 'Days' screen might occasionally not load. Disabled until tests rerun is implemented.")
@Test
fun e2eAllDayStatsLoad() {
val todayVisits = StatsVisitsData("97", "28", "14", "11")
Expand Down
2 changes: 1 addition & 1 deletion ...s/src/main/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ abstract class BaseStatsUseCase<DOMAIN_MODEL, UI_STATE>(
if (!state.cached) {
val updatedCachedData = loadCachedData()
if (domainModel != updatedCachedData) {
domainModel = updatedCachedData
domainModel = state.model
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean the whole if{} block is redundant here?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first glance, the answer is that its not redundant. Not because of my change, but because of the updateState(). If the state values are the same, I imagine we don't want to fire a call to updateState. But this is where your input will matter as there may be details about fluxc I don't fully understand. Presumably, if we are using a data class, which I believe we do for those models, data from the cache can equal data from the web. Basing this solely off the docs. I could be wrong. Your thoughts 😄 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

FluxC is also long before my time!

Wdyt? @irfano

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know what. I also just realized that I see why you are saying that. domainModel != updatedCachedData is confusing. It should be domainModel != state. Does that make more sense @ravishanker ?

Copy link
Member

Choose a reason for hiding this comment

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

You know what. I also just realized that I see why you are saying that. domainModel != updatedCachedData is confusing. It should be domainModel != state. Does that make more sense @ravishanker ?

Yes! We can remove val updatedCachedData = loadCachedData() line and check domainModel != state

updateState()
}
}
Expand Down
Loading