Skip to content

New logger implementation#4275

Merged
tobiasKaminsky merged 4 commits into
masterfrom
ezaquarii/new-logger-implementation
Aug 7, 2019
Merged

New logger implementation#4275
tobiasKaminsky merged 4 commits into
masterfrom
ezaquarii/new-logger-implementation

Conversation

@ezaquarii

Copy link
Copy Markdown
Collaborator

Fixes #4228

Signed-off-by: Chris Narkiewicz hello@ezaquarii.com

Comment thread build.gradle Outdated
Comment thread src/main/java/com/nextcloud/client/logger/FileLogHandler.kt Outdated
@nextcloud nextcloud deleted a comment Jul 28, 2019
@nextcloud nextcloud deleted a comment Jul 28, 2019
@nextcloud nextcloud deleted a comment Jul 28, 2019
@nextcloud nextcloud deleted a comment Jul 28, 2019
@nextcloud nextcloud deleted a comment Jul 28, 2019
@nextcloud nextcloud deleted a comment Jul 28, 2019
@nextcloud nextcloud deleted a comment Jul 28, 2019
Comment thread src/main/java/com/owncloud/android/ui/activity/LogHistoryActivity.java Outdated
Comment thread src/main/java/com/nextcloud/client/logger/FileLogHandler.kt Outdated
Comment thread src/main/java/com/nextcloud/client/logger/ThreadLoop.kt
Comment thread src/main/java/com/nextcloud/client/logger/ui/LogsAdapter.kt Outdated
Comment thread src/main/java/com/nextcloud/client/logger/ui/LogsAdapter.kt Outdated
@codecov

codecov Bot commented Jul 31, 2019

Copy link
Copy Markdown

Codecov Report

Merging #4275 into master will increase coverage by 0.34%.
The diff coverage is 61.63%.

@@             Coverage Diff             @@
##             master   #4275      +/-   ##
===========================================
+ Coverage     16.26%   16.6%   +0.34%     
  Complexity        1       1              
===========================================
  Files           340     351      +11     
  Lines         31184   31397     +213     
  Branches       4429    4446      +17     
===========================================
+ Hits           5071    5215     +144     
- Misses        25234   25306      +72     
+ Partials        879     876       -3
Impacted Files Coverage Δ Complexity Δ
...java/com/nextcloud/client/di/ComponentsModule.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...in/java/com/nextcloud/client/di/ViewModelModule.kt 0% <ø> (ø) 0 <0> (ø) ⬇️
.../com/nextcloud/client/logger/ui/LogsEmailSender.kt 0% <0%> (ø) 0 <0> (?)
...com/owncloud/android/ui/activity/LogsActivity.java 0% <0%> (ø) 0 <0> (?)
...owncloud/android/ui/activity/SettingsActivity.java 38.44% <0%> (ø) 0 <0> (ø) ⬇️
...va/com/nextcloud/client/logger/ui/LogsViewModel.kt 0% <0%> (ø) 0 <0> (?)
...java/com/nextcloud/client/logger/ui/LogsAdapter.kt 0% <0%> (ø) 0 <0> (?)
.../main/java/com/nextcloud/client/logger/LogEntry.kt 100% <100%> (ø) 0 <0> (?)
src/main/java/com/owncloud/android/MainApp.java 55.11% <100%> (-0.52%) 0 <0> (ø)
src/main/java/com/nextcloud/client/logger/Level.kt 100% <100%> (ø) 0 <0> (?)
... and 31 more

@nextcloud nextcloud deleted a comment Jul 31, 2019
@nextcloud nextcloud deleted a comment Jul 31, 2019
@nextcloud nextcloud deleted a comment Jul 31, 2019
@nextcloud nextcloud deleted a comment Jul 31, 2019
@nextcloud nextcloud deleted a comment Jul 31, 2019
@nextcloud nextcloud deleted a comment Jul 31, 2019
@nextcloud nextcloud deleted a comment Aug 5, 2019
@ezaquarii
ezaquarii force-pushed the ezaquarii/new-logger-implementation branch from 1d820a1 to 3155a93 Compare August 5, 2019 17:53
@nextcloud nextcloud deleted a comment Aug 5, 2019
@nextcloud nextcloud deleted a comment Aug 5, 2019
* Copyright (C) 2019 Chris Narkiewicz <hello@ezaquarii.com>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is GPL3+, an AppModule for example you used AGPL3+.
While the changes are minor, we encourage APGL3+.
(sorry if I mixed this up and falsly suggested GPL3+…)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think I copied the header from other source file - some are GPL3.

It should be all converted to AGPL3 now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Now it should be really done. (-:

@ezaquarii
ezaquarii force-pushed the ezaquarii/new-logger-implementation branch from f34c899 to 888da30 Compare August 6, 2019 14:54
@nextcloud nextcloud deleted a comment Aug 6, 2019
Fixes #4228

Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
@ezaquarii
ezaquarii force-pushed the ezaquarii/new-logger-implementation branch from 20d8cd7 to 79e8d59 Compare August 6, 2019 20:49
@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

… [skip ci]

Signed-off-by: nextcloud-android-bot <android@nextcloud.com>
@nextcloud nextcloud deleted a comment Aug 6, 2019
@nextcloud nextcloud deleted a comment Aug 6, 2019
@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

@tobiasKaminsky

Copy link
Copy Markdown
Member

https://www.kaminsky.me/nc-dev/android-integrationTests/10299/classes/com.nextcloud.client.logger.TestLogger.html

I tried to run these unit tests locally, but I always get:

java.lang.RuntimeException: Method d in android.util.Log not mocked. See http://g.co/androidstudio/not-mocked for details.

	at android.util.Log.d(Log.java)
	at com.nextcloud.client.logger.LoggerImpl.d(LoggerImpl.kt:66)
	at com.nextcloud.client.logger.TestLogger.queue overflow warning is logged(TestLogger.kt:231)

Am I missing something?

@ezaquarii

ezaquarii commented Aug 7, 2019

Copy link
Copy Markdown
Collaborator Author

@tobiasKaminsky
The only thing coming to my mind is

        testOptions {
            unitTests.returnDefaultValues = true
        }

in gradle. The above is required to enable bare-minimum system API mocks. If you restored the library dependency by restoring entire build.gradle by accident, this is what will happen.

@ezaquarii

Copy link
Copy Markdown
Collaborator Author

@tobiasKaminsky Testing platform code is tricky. I'm on the fence between having tests on android devices (androidTests) vs grabbing Robolectric. I decided to keep any extra dependencies out as long as we really don't require them.

@tobiasKaminsky

Copy link
Copy Markdown
Member

If you restored the library dependency by restoring entire build.gradle by accident, this is what will happen.

I did not (yet) changed anything, but only checked it out.
I restart drone and give it also a new clean test locally.

I decided to keep any extra dependencies out as long as we really don't require them.

Good :-) If we come to the point where we have to change this, we should really be sure about the change.

@tobiasKaminsky

Copy link
Copy Markdown
Member

Interesting. Running TestLogger via Android Studio results in error above.

But running "./gradlew testGplayDebugUnitTest" says that everything works:

com.nextcloud.client.logger.TestLogger > queue overflow warning is logged PASSED

--> so false alarm, but still strange

@tobiasKaminsky

Copy link
Copy Markdown
Member

After CI, I will merge lib, and do all the needed stuff to get this in.

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@nextcloud nextcloud deleted a comment Aug 7, 2019
@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10311.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Codacy

278

Lint

TypemasterPR
Warnings5959
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings24
Correctness Warnings69
Internationalization Warnings12
Malicious code vulnerability Warnings4
Multithreaded correctness Warnings9
Performance Warnings111
Security Warnings46
Dodgy code Warnings135
Total410

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings24
Correctness Warnings69
Internationalization Warnings12
Malicious code vulnerability Warnings4
Multithreaded correctness Warnings9
Performance Warnings112
Security Warnings46
Dodgy code Warnings136
Total412

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.

Log_OC doesn't collect logs in file

4 participants