Conversation
|
You can test the changes on this Pull Request by downloading the APK here. |
367d34d to
dc30c1d
Compare
dc30c1d to
6216552
Compare
The main project uses 21, so this shouldn’t be a problem
* Removes `this` * Makes the `File` `@TestOnly`
oguzkocer
left a comment
There was a problem hiding this comment.
We did a review on call with @jkmassel and made some changes to it in the suggesion/add-log-persistence branch. (sorry about the spelling error) The major point of these improvements were de-coupling things from each other as much as we could.
db9edda to
f43c4f6
Compare
f43c4f6 to
29a0ea5
Compare
libs/utils/WordPressUtils/src/main/java/org/wordpress/android/util/AppLog.java
Outdated
Show resolved
Hide resolved
| * | ||
| * If the directory doesn't already exist, it will be created. | ||
| */ | ||
| override fun getLogFileDirectory(): File { |
There was a problem hiding this comment.
This is super unimportant, but as we also talked about it on the screenshare session, I prefer to avoid doing multiple things in a function when I can avoid it. In this case avoiding it may not be the best strategy, so in these cases, I do something like this:
override fun getLogFileDirectory(createDirectoryIfNecessary: Boolean = true): File? {
In this case making this return a nullable has a cascading effect and I don't think we should do it, but I thought I'd mention this strategy for future reference. I think this type of approach makes the code more self-documenting.
P.S: I wish we could specify a return type or just the nullability depending on a parameter.
There was a problem hiding this comment.
We don't have to do a nullable return here, we could just use a check to validate that the directory can be created.
The issue we hit is that it seems we can't use a default parameter in an overriding method for an interface. And an interface can't specify default parameter values.
So I think we'd have to rework quite a few things to make this work, unfortunately.
There was a problem hiding this comment.
The nullable return would have been necessary because if the createDirectoryIfNecessary in that hypothetical function was false, we'd have to return null when the directory doesn't exist, right?
There was a problem hiding this comment.
Ah, yeah I suppose so. That's a weird API IMHO because it lets you really paint yourself into a corner though 🤔
There's probably a better way to handle it, but it's not coming to me offhand – I think we could leave this for a future change.
There was a problem hiding this comment.
Definitely agreed, and that's why in my initial comment I said this was mostly mentioned for a future reference. I don't think there is a good way to do this for this feature, at least not in its current state.
And update the references
4ce7a80 to
b7b7d2d
Compare
...ils/WordPressUtils/src/main/java/org/wordpress/android/util/helpers/logfile/LogFileWriter.kt
Outdated
Show resolved
Hide resolved
| @RunWith(RobolectricTestRunner::class) | ||
| @Config(sdk = [Build.VERSION_CODES.O_MR1]) | ||
| class LogFileCleanerTest { | ||
| lateinit var logFileProvider: LogFileProvider |
There was a problem hiding this comment.
Since the context we are using doesn't seem to change, I think we can use a val here instead.
Also, it should be private regardless of this change :)
private val logFileProvider = LogFileProvider.fromContext(ApplicationProvider.getApplicationContext())
There was a problem hiding this comment.
Fixed the private thing, in 3185b16. It does change because the setup method runs before every test case, so this has to be a var so that it can be reinitialized each time.
There was a problem hiding this comment.
The context won't change though, so the LogFileProvider created from it wouldn't change either. Am I missing something? 🤔
libs/utils/WordPressUtils/src/test/java/org/wordpress/android/util/LogFileCleanerTest.kt
Outdated
Show resolved
Hide resolved
libs/utils/WordPressUtils/src/test/java/org/wordpress/android/util/LogFileCleanerTest.kt
Outdated
Show resolved
Hide resolved
libs/utils/WordPressUtils/src/test/java/org/wordpress/android/util/LogFileCleanerTest.kt
Outdated
Show resolved
Hide resolved
And fix a typo
libs/utils/WordPressUtils/src/test/java/org/wordpress/android/util/LogFileCleanerTest.kt
Outdated
Show resolved
Hide resolved
libs/utils/WordPressUtils/src/test/java/org/wordpress/android/util/LogFileHelpersTest.kt
Outdated
Show resolved
Hide resolved
libs/utils/WordPressUtils/src/test/java/org/wordpress/android/util/LogFileHelpersTest.kt
Outdated
Show resolved
Hide resolved
libs/utils/WordPressUtils/src/test/java/org/wordpress/android/util/LogFileHelpersTest.kt
Show resolved
Hide resolved
| @RunWith(RobolectricTestRunner::class) | ||
| @Config(sdk = [Build.VERSION_CODES.O_MR1]) | ||
| class LogFileWriterTest { | ||
| lateinit var testProvider: LogFileProvider |
There was a problem hiding this comment.
Depending on what we end up doing with #11536 (comment) we should follow the same thing here. At the very least, it should be private.
There was a problem hiding this comment.
Changed this to private in a29f8bb.
Same comment about mutability and this one has the added complexity that it's important to clean up the test directory after to avoid shared state.
- Makes testProvider private - DRY file creation code - Separates file name, creation date, and modification date so that over test runs, none will be the same
1a43e016c6 Merge pull request #29 from wordpress-mobile/issue/28-empty-img-tag-crash-fix a4c24373e5 Updated the release version to 1.24 a33296f4d6 Check if img source is null before checking for image smileys 1dd7406ad5 Merge pull request #27 from wordpress-mobile/combined-updates-from-wpandroid b918b23ac5 Check if announcement is available in App Settings before showing option. Updated AppLog to include announcement related tracker. 94e23e28c1 Rename build.gradle fields with invalid name syntax d0be1240ca Make sure url utils recognizes atomic image proxy as valid image url. 9bdbdff3a9 Merge branch 'feature/support-private-at-sites' of https://github.com/wordpress-mobile/WordPress-Android into feature/support-private-at-sites fcc5c99e37 Updated Photon utils to utilize new url for private AT proxy. 130195092f Gutenberg/integrate release 1.25.0 with dark mode (#11580) a25096c291 Restoring Utils lib version to 1.24 edfef1e58c Merge branch 'develop' into issue/11249-media-not-load-jetpack 60d516266e Merge pull request #11536 from wordpress-mobile/add/log-persistence 1aceca9950 Make the linter happy 18c8447fe3 Change provider visibility dac4e5ce38 Fixes to LogFileHelpersTest 76f9f75f5d Variablize recent created files test a4d0a773e7 Small fixes for LogFileCleanerTest 24ac53fabf Make the queue private 64ea6a8cdf Use correct hungarian notation for static member 1c978ca61a Refactor tests to match project refactoring 565b88903c Persist log files in a background thread 3ae86d48e4 Refactor LogEntry.toString method b900bdda83 Refactor LogFileHelpers to not use context and be a LogFileProvider 225e1db645 Upgrading lib version. 6fd47e8eb6 Using the assertThat pattern. 0948a93056 Adding connected testing. dc5c2e3cd6 Refactor LogFileWriter to use LogFileProvider instead of context 2d5d0b65c0 Refactor LogFileCleaner to not need context and use a LogFileProvider instead 84b7a8a87d Minor changes in LogFileWriter to make it more idiomatic 8cfa0950aa Merge branch 'develop' into issue/11249-media-not-load-jetpack 31a14daeb4 Bump the utils minSdkVersion to 18 f65fd78414 Add log persistence fe3960d6b7 Updating ssl logic for photon addresses. 1b4b31bf88 Merge pull request #11524 from wordpress-mobile/fix/wordpressutils-tests 6c80419806 Fix WordPressUtils tests def5be4e88 Revert "Feature/material theme and Dark Theme support (#11469)" (#11486) 16540e85f1 Feature/material theme and Dark Theme support (#11469) 0ae297bbd3 Adding ssl parameter to query for Photon https. 89725645a6 made method name more meaningful. 76343d3d6d Added util function to check if a Uri is a content one. 75790052ef Merge pull request #11072 from wordpress-mobile/issue-10843/latitude_fix_api_29 981eb82158 Merge branch 'develop' into try/media-upload-completion-processor 8799a19ec4 removes safe check due to nature of RecyclerView 7906f07645 Fixed latitude missing column issue on api 29 882a64faf3 updated gradle wrapper of utils project. b012d38c3f resolved conflicts and merged utils subtree e8aae6d282 Merge branch 'develop' into try/media-upload-completion-processor da19376391 Add MediaFile method to generate attachment page url e35a9e0cd8 Used delegate safe method for focused accessibility event listener de02499434 Merge remote-tracking branch 'origin/develop' into issue/10894-aztec-media-talkback 1812393e22 Added accessibility heading 30aa9a3132 Added a safe way to set accessibility delegates. 870518ff07 Moved method to util class that's more appropriate for it's behavior a998f6e877 added annotations to event listener method 43013a5a79 Moved behavior to Utils so that it can be replicated in other areas. 54ca204429 Removed unused imports. 373db7c13f Removed unneeded whitespace and added necessary whiteline 785da6617b WP Media Picker now announces when an image is selected. 75f0913259 "Done" button now has a content description of "Cancel" 42ec1fb96c Disabled hint announcements and applied accessibility headings 916e2b0d96 Add comment to downloadExternalMedia method 573ba7f5e8 Clean up AddMediaToEditor logic dda3a1ebb8 Merge pull request #10565 from wordpress-mobile/clean-up-after-legacy-editor d84fb1f61f Upgrade Gradle to 5.4.1, gradle plugin to 3.5.1 and fix various errors 0511354994 Remove WPEditText 19d3f4aaf0 Optimize AppLog b4401cd84f Fix simple date format concurrency issue 15ee1b27d0 Remove usages of buggy DateTimeUtils methods e9f01e5e3c Fixed loop. 47eec63356 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9815-email-verification-reminder 25d1310312 Fix AndroidX import order a21c7120d8 Fix import ordering for androidx 35b99bc73e Migrate to AndroidX d40e8773bb Merge branch 'issue/9815-create-call-to-action' into issue/9815-add-reminder-message 892eacce49 Make getPath private again ae6291c612 Use existing method instead of creating a new one in MediaUtils 10b59f8d06 Implement comments from PR f1b21d0acc Add logging for the failing cases 702881b26f A crash fix for uploaded docs cc1c0334a8 Added reminder message toast after domain has been registered 9a14564929 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions a4d8873a2e Update style config from style-config-android 1c10935b8c Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions 971cf76d96 Remove unused get snackbar duration method from accessibility utils class 28b0685ae7 Update is accessibility enabled method in accessibility utils for simplicity a418cb7dc8 Update get snackbar duration method in accessibility utils for indefinite constant e0dd4824c9 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions 506f9883ce refactored ToastUtils to be able to pass gravity as param. Also showing toast after back-dating a scheduled post on top anchor now e164cd7a61 Fixing merge conflicts from domain register branch. 586222ec51 Merge pull request #8164 from usamahamid/feature/domain-register 50de29e20a Introduced Domain Suggestion Network request in ViewModel git-subtree-dir: libs/utils git-subtree-split: 1a43e016c63bd9dc19f3e27d39b8dc1cba7c5263
To test:
PR submission checklist:
RELEASE-NOTES.txtif necessary.