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

Add logging for all known migration errors #17748

Merged
merged 5 commits into from
Jan 19, 2023

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Jan 12, 2023

Fixes #17746

Related PR: wordpress-mobile/WordPress-Utils-Android#119

To test:

  1. Install the WordPress app (but do not log in)
  2. Install the Jetpack app
  3. Run the Jetpack app and observe logcat
  4. Expect that the failure to migrate will be logged as an error with the tag "JETPACK_MIGRATION"

Bonus:

Manually cause a failure by emitting an error from a different migration step (e.g. sites)

Expect the error to be logged as well.

Regression Notes

  1. Potential unintended areas of impact
    Migration

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual and automated tests (including migration unit tests)

  3. What automated tests I added (or what prevented me from doing so)
    Adding tests for AppLog and AppLogWrapper proved to be time-consuming and problematic. The AppLog used in unit tests is different from the one used in production (to avoid calls to Android Framework methods), and the wrapper is not resolving correctly, resulting in binary incompatibility. It might be worth improving the architecture of the logging wrapper(s) to make adding tests more ergonomic. Currently, it seems that only mocking is supported, but actually verifying anything on those mocks results in classpath related build errors.

Update: I found a solution to the verify issue by providing a simple test double for AppLogWrapper. Usually, a wrapper would allow mocking (and verifying) directly, but this is needed to satisfy the type bindings in the test environment because we are already doing the same for AppLog (a shared dependency).

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.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 12, 2023

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17748-57cb95f.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commit57cb95f
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 12, 2023

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17748-57cb95f.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commit57cb95f
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

The solution used in this commit is to copy the logging wrapper from
fluxc in the unit test source path so that it remains compatible with
the copied AppLog class (which is also copied in a similar manner, as a
work-around). This prevents build errors when the logging wrapper's mock
is verified to have been invoked.
@ovitrif ovitrif self-requested a review January 16, 2023 10:59
Copy link
Contributor

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this @mkevins 🙇

I tested the following scenarios:

  • ✅ (testing steps) Start JP app with WP not logged in → error is logged
  • ✅ Create local draft in WP app → migration fall back to login → logged error:
    E/WordPress-JETPACK_MIGRATION: Ineligibility(reason=LocalDraftContentIsPresent)
    
  • ✅ Manually emit error during user flags migration → migration error screen → logged error :
    E/WordPress-JETPACK_MIGRATION: org.wordpress.android.localcontentmigration.LocalMigrationError$NoUserFlagsFoundError@10cf5a2
    

The enhanced logging works as expected 👍

PS: Nice solution to mitigate the unit testing issues 😅 🏅

Feel free to merge this if you confirm it's ready :shipit:

@mkevins mkevins enabled auto-merge January 19, 2023 02:56
@wpmobilebot
Copy link
Contributor

Found 1 violations:

The PR caused the following dependency changes:

-\--- org.wordpress:utils:{strictly 3.1.0} -> 3.1.0
+\--- org.wordpress:utils:{strictly trunk-1ec18489fc79bb8f10b827586f6efcdf3c13f5b1} -> trunk-1ec18489fc79bb8f10b827586f6efcdf3c13f5b1

Please review and act accordingly

@mkevins mkevins merged commit 489e062 into trunk Jan 19, 2023
@mkevins mkevins deleted the try/add-logging-for-all-migration-errors branch January 19, 2023 03:11
@mkevins
Copy link
Contributor Author

mkevins commented Jan 19, 2023

Thanks Ovi for testing and reviewing!

PS: Nice solution to mitigate the unit testing issues sweat_smile medal_sports

Thanks, I was glad to find a solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android - Add better logging for migration errors
3 participants