cleanup appstate hook, refreshTimeline on app resume, revamp clientstats #1179
Conversation
TimelineContext When appstate resumes from background, refresh the timeline to avoid stale state useAppStateChange: Added stat reading and removed log statement (because the stat reading itself performs a log) Clean up the subscription on teardown (the return of useEffect) clientStats: Updated the stat keys to be typed strings, eliminating the need for statKeys. `'my_stat_key'` is less verbose than importing statKeys and calling`statKeys.MY_STAT_KEY`, and this approach utilizes Typescript. Updated existing clientstats readings and removed unused clientstats keys. Added new clientstats readings in useAppStateChange MultiLabelButtonGroup. Unified CLIENT_NAV_EVENT and CLIENT_TIME since they are fundamentally the same and only differ by whether the 'reading' field has a value or is null
JGreenlee
force-pushed
the
appstate-clientstats
branch
3 times, most recently
from
6c25251
to
037d69c
Compare
Whenever the user sees an error, it will be recorded in clientStats as a `stats/client_error`
JGreenlee
force-pushed
the
appstate-clientstats
branch
from
037d69c
to
f8fcf10
Compare
|
Not sure what is going on with the checks. Failed once and it seemed like some tests were not able to run. |
|
@JGreenlee tests are now running, but all dynamic config tests are failing because we are trying to read some cordova value It is related to |
in dynamicConfig.test.ts, there are some tests where an error is expected to be thrown and error popup is triggered. clientstats is invoked because we are now recording a ui_error stat on all error popups. Client stats needs mockDevice and mockGetAppVersion to work in startprefs.test.ts, a similar error popup occurs so the mocks are needed there too. However, while inspecting startprefs.ts, I noticied the popup occurs when consent document exists in localstorage but not in native storage. I do not think we need an error popup for this; instead let's do what we do in storage.ts, which is record a 'missing_keys" client stat, and give it a type of 'document_mismatch'.
Instead of having each test file call the mocks it uses, we can have jest set up all the mocks beforehand. in jest.config.js, setupFiles points to setupJestEnv.js where all the mocks are called. This is way less verbose, and may also speed up the execution of the test suites because the mocks are applied once rather than duplicated for every individual test. We still have flexibility for individual tests to call mocks on their own; for example in enketoHelper.test.ts there are several calls to mockBEMUserCache(<fakeconfig>) because it is testing out various configs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1179 +/- ##
==========================================
+ Coverage 30.13% 30.14% +0.01%
==========================================
Files 118 118
Lines 5164 5172 +8
Branches 1108 1110 +2
==========================================
+ Hits 1556 1559 +3
- Misses 3606 3611 +5
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
fix tests, record client stats for missing native consentin dynamicConfig.test.ts, there are some tests where an error is expected to be thrown and error popup is triggered. clientstats is invoked because we are now recording a ui_error stat on all error popups. in startprefs.test.ts, a similar error popup occurs so the mocks are needed there too. simplify mocking by specifying jest setup fileInstead of having each test file call the mocks it uses, we can have jest set up all the mocks beforehand. This is way less verbose, and may also speed up the execution of the test suites because the mocks are applied once rather than duplicated for every individual test. We still have flexibility for individual tests to call mocks on their own; for example in enketoHelper.test.ts there are several calls to mockBEMUserCache() because it is testing out various configs |
Fair. We should record both mismatches (localstorage but not native storage, and native storage but not localstorage) as errors because that is one of the hardest issues to debug. |
TimelineContext
When appstate resumes from background, refresh the timeline to avoid stale state
useAppStateChange:
Added stat reading and removed log statement (because the stat reading itself performs a log) Clean up the subscription on teardown (the return of useEffect)
clientStats:
Updated the stat keys to be typed strings, eliminating the need for statKeys.
'my_stat_key'is less verbose than importing statKeys and callingstatKeys.MY_STAT_KEY, and this approach utilizes Typescript. Updated existing clientstats readings and removed unused clientstats keys. Added new clientstats readings in useAppStateChange MultiLabelButtonGroup. Unified CLIENT_NAV_EVENT and CLIENT_TIME since they are fundamentally the same and only differ by whether the 'reading' field has a value or is null