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

notandyvee · 2024-04-06T00:35:37Z

There was a failing UI test that appeared flaky, StatsGranularTabsTest. Retrying appears to fix it. Every once in a while I also notice stats cards don't load properly. It's super hard to reproduce, even in UI tests.

At first I figured out part of the issue. I noticed each failing stats card would have an empty BlockListItem list. Each card can have multiple BlockListItems. When the StatsBlock has no BlockListItem, it can't do anything, so presents itself as a card with nothing in it. This is unfortunate, but not really an issue. The real issue is why a StatsBlock can even have an empty list.

After further testing I learned that what leads to this issue is actually a race condition. The UI is dependent on setting the domainModel in BaseStatsUseCase. In most cases, the domain model is present quickly when we load the UI. But during UI tests situations arise where the domainModel takes a bit longer to load, leading to a race condition. As far as I can tell, it really only happens (or noticeable) when you first login or switch to a blog for the first time. The change I made in BaseStatsUseCase best illustrates why I think this fixes the issue.

When a user first loads stats, there is no cache. So it must fetch from the network. When it does that, the domainModel is set to the cache, which technically doesn't exist yet. This happens in evaluateState when a network call is made.

domainModel = updatedCachedData

It's not entirely clear to me why this is being done. In evaluateState if we got state back, presumably we should use that as it would be the most up-to-date. So I just tried it.

domainModel = state.model

I re-ran the previously failing UI test dozens of times. Couldn't reproduce it. I also 🚬 tested stats and don't notice any differences.

To Test:

StatsGranularTabTest must pass 100% of the time. If on first pass it fails, this PR is pointless 😅 .
🚬 test granular stats. Make sure you load all cards and that everything looks as it did before.

Regression Notes

Potential unintended areas of impact

Stats cards loading.
What I did to test those areas of impact (or what existing automated tests I relied on)

Manual test and UI test.
What automated tests I added (or what prevented me from doing so)

n/a

PR Submission Checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…all.

wpmobilebot · 2024-04-06T00:57:20Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20606-71d7ad3
	Commit	`71d7ad3`
	Direct Download	`wordpress-prototype-build-pr20606-71d7ad3.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-04-06T01:02:41Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20606-71d7ad3
	Commit	`71d7ad3`
	Direct Download	`jetpack-prototype-build-pr20606-71d7ad3.apk`

Note: Google Login is not supported on these builds.

codecov · 2024-04-06T01:06:45Z

Codecov Report

Attention: Patch coverage is 40.74074% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 40.48%. Comparing base (17f5845) to head (32754de).
Report is 73 commits behind head on trunk.

Files	Patch %	Lines
...ns/insights/usecases/MostPopularInsightsUseCase.kt	25.00%	0 Missing and 6 partials ⚠️
...sts/sections/granular/usecases/ReferrersUseCase.kt	16.66%	4 Missing and 1 partial ⚠️
...ions/insights/usecases/LatestPostSummaryUseCase.kt	50.00%	0 Missing and 2 partials ⚠️
...i/stats/refresh/lists/sections/BaseStatsUseCase.kt	66.66%	0 Missing and 1 partial ⚠️
...dget/configuration/StatsColorSelectionViewModel.kt	66.66%	0 Missing and 1 partial ⚠️
...idget/configuration/StatsSiteSelectionViewModel.kt	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #20606      +/-   ##
==========================================
- Coverage   40.50%   40.48%   -0.03%     
==========================================
  Files        1474     1474              
  Lines       67961    67979      +18     
  Branches    11235    11246      +11     
==========================================
- Hits        27529    27519      -10     
- Misses      37949    37966      +17     
- Partials     2483     2494      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ravishanker · 2024-04-08T02:29:49Z

...ress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCase.kt

@@ -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


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

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 😄 ?

FluxC is also long before my time!

Wdyt? @irfano

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 ?

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

irfano

When a user first loads stats, there is no cache. So it must fetch from the network. When it does that, the domainModel is set to the cache, which technically doesn't exist yet.

This is not entirely accurate. FluxC writes to the cache before sending responses to the WordPress-Android. So, before val updatedCachedData = loadCachedData() line, it is already fetched and the cache has existed. But I do agree with your final suggestion.

It's not entirely clear to me why this is being done. In evaluateState if we got state back, presumably we should use that as it would be the most up-to-date. So I just tried it.

I agree!

I tested trunk and this PR multiple times and have never encountered the issue on this PR while I could reproduce it on trunk. Additionally, I compared the content of all the cards between PR and the trunk version, and they were identical.
Andy, you can address Ravi's comment, and then I believe we're good to merge. 🟢

irfano · 2024-04-09T15:52:43Z

...ress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCase.kt

@@ -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


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

notandyvee · 2024-04-09T18:42:23Z

Thanks for adding the missing context @irfano . Very useful information. And actually makes me wonder why we don't just listen to the database cache updates directly and not deal with the network at all 😅 . But that's out of scope for this PR. I went ahead and made that change. Ended up removing the if like Ravi said anyways.

notandyvee · 2024-04-09T20:30:47Z

Damn. Heads up @irfano . That seemingly small change causes a few new failed tests. Some of them are expected. But others are tricky, like this one. I need to slowly go through it all and ensure I am not introducing a regression 😅 .

ravishanker

Great work 🚀

notandyvee · 2024-04-11T20:42:17Z

Hey @irfano when you get a chance, no rush. Quick re-review. I made the changes we talked about. But had to update a bunch of unit tests. I may have went down a rabbit hole. But the error wasn't clear here, so I went the extra mile and updated the unit tests so they pass locally without the need to change the flavor, which is more proper anyways.

Note: Most of the files edited were test files. Small exceptions.

...src/test/java/org/wordpress/android/ui/stats/refresh/lists/detail/PostDayViewsUseCaseTest.kt

.../src/test/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCaseTest.kt

...g/wordpress/android/ui/stats/refresh/lists/sections/granular/usecases/OverviewUseCaseTest.kt

.../wordpress/android/ui/stats/refresh/lists/sections/granular/usecases/ReferrersUseCaseTest.kt

...rdpress/android/ui/stats/refresh/lists/sections/insights/usecases/AllTimeStatsUseCaseTest.kt

WordPress/build.gradle

notandyvee · 2024-04-15T16:13:39Z

Ready for re-review @irfano . Let me know if you have any additional questions. All good stuff as we should questions the changes 🙏🏻

irfano · 2024-04-16T13:22:14Z

Thank you for bearing my comments, @notandyvee. 😅 I'm curious about your opinion on my two open comments. Then we're good to merge.

sonarqubecloud · 2024-04-16T19:11:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

irfano

LGTM! 👍🏻 Thank you for your contributions to improve our tests, Andy. Great work! 🚀

Andy Valdez added 2 commits April 5, 2024 19:49

[Bug] Fixed bug where stats card doesn't always show up on clean inst…

b20ce69

…all.

[Test] Uncomment previously failing UI test.

619252f

notandyvee added the [Type] Bug label Apr 6, 2024

notandyvee added this to the 24.7 milestone Apr 6, 2024

notandyvee requested review from ravishanker and irfano April 6, 2024 00:35

notandyvee requested a review from a team as a code owner April 6, 2024 00:35

ravishanker reviewed Apr 8, 2024

View reviewed changes

Merge branch 'trunk' into andy/wp-android-issue-18065

9275240

irfano approved these changes Apr 9, 2024

View reviewed changes

Andy Valdez added 2 commits April 9, 2024 14:29

[Bug] Updated the state check and removed uneccessary cache load.

1fd534f

[Bug] Remove an if level.

927303a

ravishanker approved these changes Apr 9, 2024

View reviewed changes

Update unit test that I noticed were failing.

21c1721

notandyvee modified the milestones: 24.7, 24.8 Apr 11, 2024

irfano requested changes Apr 12, 2024

View reviewed changes

Rename some tests and removed test lib I accidentally added.

32754de

Andy Valdez added 2 commits April 16, 2024 13:01

Merge branch 'trunk' into andy/wp-android-issue-18065

1ecc095

Make further review updates.

71d7ad3

irfano approved these changes Apr 17, 2024

View reviewed changes

irfano merged commit 3e4efdb into trunk Apr 17, 2024
20 checks passed

irfano deleted the andy/wp-android-issue-18065 branch April 17, 2024 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

notandyvee commented Apr 6, 2024

wpmobilebot commented Apr 6, 2024 •

edited

Loading

wpmobilebot commented Apr 6, 2024 •

edited

Loading

codecov bot commented Apr 6, 2024 •

edited

Loading

ravishanker Apr 8, 2024

notandyvee Apr 8, 2024

ravishanker Apr 9, 2024

notandyvee Apr 9, 2024

irfano Apr 9, 2024

irfano left a comment •

edited

Loading

irfano Apr 9, 2024

notandyvee commented Apr 9, 2024

notandyvee commented Apr 9, 2024

ravishanker left a comment

notandyvee commented Apr 11, 2024 •

edited

Loading

notandyvee commented Apr 15, 2024

irfano commented Apr 16, 2024

sonarqubecloud bot commented Apr 16, 2024

irfano left a comment

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

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

Conversation

notandyvee commented Apr 6, 2024

To Test:

Regression Notes

PR Submission Checklist:

wpmobilebot commented Apr 6, 2024 • edited Loading

wpmobilebot commented Apr 6, 2024 • edited Loading

codecov bot commented Apr 6, 2024 • edited Loading

Codecov Report

ravishanker Apr 8, 2024

Choose a reason for hiding this comment

notandyvee Apr 8, 2024

Choose a reason for hiding this comment

ravishanker Apr 9, 2024

Choose a reason for hiding this comment

notandyvee Apr 9, 2024

Choose a reason for hiding this comment

irfano Apr 9, 2024

Choose a reason for hiding this comment

irfano left a comment • edited Loading

Choose a reason for hiding this comment

irfano Apr 9, 2024

Choose a reason for hiding this comment

notandyvee commented Apr 9, 2024

notandyvee commented Apr 9, 2024

ravishanker left a comment

Choose a reason for hiding this comment

notandyvee commented Apr 11, 2024 • edited Loading

notandyvee commented Apr 15, 2024

irfano commented Apr 16, 2024

sonarqubecloud bot commented Apr 16, 2024

Quality Gate passed

irfano left a comment

Choose a reason for hiding this comment

wpmobilebot commented Apr 6, 2024 •

edited

Loading

wpmobilebot commented Apr 6, 2024 •

edited

Loading

codecov bot commented Apr 6, 2024 •

edited

Loading

irfano left a comment •

edited

Loading

notandyvee commented Apr 11, 2024 •

edited

Loading