Fixes part of #1433: Including Console Logger into Oppia Logger

[Arjupta]

Explanation

Fixes part of #1433: Restructure the Logger
Using OppiaLogger as the central logging tool

Few files stil depend on Console Logger due to folllowing reasons -

1. Circular Dependency Graph

   • src/main/java/org/oppia/android/domain/oppialogger/analytics/AnalyticsController.kt
   • src/main/java/org/oppia/android/domain/oppialogger/exceptions/ExceptionsController.kt
   • src/main/java/org/oppia/android/domain/oppialogger/loguploader/LogUploadWorker.kt
   • src/main/java/org/oppia/android/domain/oppialogger/exceptions/UncaughtExceptionLoggerStartupListener.kt

2. Module not depends on domain

   • src/main/java/org/oppia/android/util/caching/AssetRepository.kt

Checklist

• The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
• The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
• The PR follows the style guide.
• The PR does not contain any unnecessary auto-generated code from Android Studio.
• The PR is made from a branch that's not called "develop".
• The PR is made from a branch that is up-to-date with "develop".
• The PR's branch is based on "develop" and not on any other branch.
• The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

[vinitamurthi]

Approach LGTM, however there seems to be a build error due to which domain tests are failing. Can you check that?

[rt4914]

Approving this for codeowner files only.
Thanks

[anandwana001]

my code owners files LGTM

[Arjupta]

Approach LGTM, however there seems to be a build error due to which domain tests are failing. Can you check that?

@vinitamurthi PTAL now

[vinitamurthi]

LGTM, thanks!

[Arjupta]

@BenHenning can you confirm the changes and merge this PR? Thanks

[Sarthak2601]

Thanks @Arjupta. The implementation LGTM. However, I noticed you haven't added tests for the console logger methods in the OppiaLoggerTest file. Is there something I'm missing here?

[BenHenning]

Thanks @Arjupta. Had just one comment, but I other agree with @Sarthak2601's earlier comment regarding tests: we should make sure the new functionality being added to OppiaLogger is being tested.

[Arjupta]

Thanks @Arjupta. Had just one comment, but I other agree with @Sarthak2601's earlier comment regarding tests: we should make sure the new functionality being added to OppiaLogger is being tested.

I too agree that we should encorporate tests for newly added implementations. But the only job of oppia logger here is to call the public methods that are defined in Console Logger. This is similar to calling the logException and logEvent methods of analytics controller, which is currently not being tested. So I think it is better to have tests for the ConsoleLogger itself rather than for the Oppia Logger. Other wise if we still need to test Oppia Logger for these changes can you give a hint how should I move forward. @BenHenning @Sarthak2601

[Sarthak2601]

Thanks @Arjupta. Had just one comment, but I other agree with @Sarthak2601's earlier comment regarding tests: we should make sure the new functionality being added to OppiaLogger is being tested.

I too agree that we should encorporate tests for newly added implementations. But the only job of oppia logger here is to call the public methods that are defined in Console Logger. This is similar to calling the logException and logEvent methods of analytics controller, which is currently not being tested. So I think it is better to have tests for the ConsoleLogger itself rather than for the Oppia Logger. Other wise if we still need to test Oppia Logger for these changes can you give a hint how should I move forward. @BenHenning @Sarthak2601

Makes sense, but the only difference here is that Console Logger doesn't have tests of its own whereas the Analytics Controller does. We can make use of the fake implementation pattern to achieve this. Deferring to @BenHenning here.

[Arjupta]

@vinitamurthi can you check if these tests Looks good to you. @BenHenning might be unavailable

[vinitamurthi]

Tests LGTM!

[Arjupta]

@BenHenning PTAL

[Sarthak2601]

I think we can actually test this without using a fake. Robolectric supports retrieving logged messages via ShadowLog: http://robolectric.org/javadoc/3.0/org/robolectric/shadows/ShadowLog.html.

Regarding testing OppiaLogger vs. ConsoleLogger, we should ideally test both since both have public APIs. That being said, OppiaLogger is probably more important since it's the API we're now exposing everywhere (ConsoleLogger could disappear and OppiaLogger should still behave correctly).

Might that work @Sarthak2601 / @Arjupta?

SGTM.

[BenHenning]

Thanks @Arjupta! I think the new tests look quite good. Just had a few comments.

[Arjupta]

@BenHenning can you look at these tests. I don't know why applying the sugested changes is creating such havoc. Also i was not able to run the OppiaLogger Test because of this issue.

Exception in thread "main" java.lang.annotation.AnnotationFormatError: Invalid default: public abstract java.lang.Class org.robolectric.annotation.Config.application()

[BenHenning]

@Arjupta I think the Bazel CI issues are due to #3157. The changes seem to be passing on Gradle.

Could you provide more context on the issue you're facing? A full log of the build stack trace would be helpful.

[Arjupta]

@Arjupta I think the Bazel CI issues are due to #3157. The changes seem to be passing on Gradle.

Could you provide more context on the issue you're facing? A full log of the build stack trace would be helpful.

The complete log is like this

Exception in thread "main" java.lang.annotation.AnnotationFormatError: Invalid default: public abstract java.lang.Class org.robolectric.annotation.Config.application()
at java.lang.reflect.Method.getDefaultValue(Method.java:612)
at sun.reflect.annotation.AnnotationType.<init>(AnnotationType.java:132)
at sun.reflect.annotation.AnnotationType.getInstance(AnnotationType.java:85)
at sun.reflect.annotation.AnnotationParser.parseAnnotation2(AnnotationParser.java:266)
at sun.reflect.annotation.AnnotationParser.parseAnnotations2(AnnotationParser.java:120)
at sun.reflect.annotation.AnnotationParser.parseAnnotations(AnnotationParser.java:72)
at java.lang.Class.createAnnotationData(Class.java:3521)
at java.lang.Class.annotationData(Class.java:3510)
at java.lang.Class.getAnnotation(Class.java:3415)
at com.intellij.junit4.JUnit4TestRunnerUtil.buildRequest(JUnit4TestRunnerUtil.java:219)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:46)
at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:230)
at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:58)

Process finished with exit code 1

[Arjupta]

@BenHenning PTAL. The tests are passing now

[rt4914]

LGTM

Dismissing as per chat this can be approved now.