mozilla-mobile / firefox-echo-show

Firefox for Amazon's Echo Show
Mozilla Public License 2.0
25 stars 12 forks source link

Closes #331: Update pro-guard setting for glean #333

Closed psymoon closed 4 years ago

psymoon commented 4 years ago

Pull Request checklist

codecov-commenter commented 4 years ago

Codecov Report

Merging #333 into master will increase coverage by 0.58%. The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #333      +/-   ##
============================================
+ Coverage     21.33%   21.91%   +0.58%     
- Complexity      126      127       +1     
============================================
  Files            92       92              
  Lines          1903     1921      +18     
  Branches        297      297              
============================================
+ Hits            406      421      +15     
- Misses         1459     1462       +3     
  Partials         38       38              
Impacted Files Coverage Δ Complexity Δ
...rg/mozilla/focus/telemetry/DataUploadPreference.kt 66.66% <50.00%> (-3.93%) 4.00 <0.00> (ø)
...rc/main/java/org/mozilla/focus/FocusApplication.kt 82.35% <91.66%> (+5.08%) 4.00 <1.00> (+1.00)
...va/org/mozilla/focus/telemetry/TelemetryWrapper.kt 19.59% <100.00%> (+1.10%) 3.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0e7ecfa...3306b15. Read the comment docs.

mcomella commented 4 years ago

Some initial thoughts: I'm concerned about the -dontwarn line, that we may suppress something critical.

I think what's happening is a few GleanTest* classes are in the production dependency and their reference junit classes which are not available. If I change the junit rule to implementation, I get a different set of warnings:

Warning: library class android.test.AndroidTestCase extends or implements program class junit.framework.TestCase                                                                   
Warning: library class android.test.AndroidTestRunner extends or implements program class junit.runner.BaseTestRunner                                                              
Warning: library class android.test.InstrumentationTestCase extends or implements program class junit.framework.TestCase                                                           
Warning: library class android.test.InstrumentationTestSuite extends or implements program class junit.framework.TestSuite                                                         
Warning: library class android.test.suitebuilder.TestSuiteBuilder$FailedToCreateTests extends or implements program class junit.framework.TestCase                                 
Warning: org.junit.internal.runners.statements.FailOnTimeout: can't find referenced class java.lang.management.ManagementFactory                                                   
Warning: org.junit.internal.runners.statements.FailOnTimeout: can't find referenced class java.lang.management.ThreadMXBean                                                        
Warning: org.junit.internal.runners.statements.FailOnTimeout: can't find referenced class java.lang.management.ThreadMXBean                                                        
Warning: org.junit.internal.runners.statements.FailOnTimeout: can't find referenced class java.lang.management.ManagementFactory                                                   
Warning: org.junit.internal.runners.statements.FailOnTimeout: can't find referenced class java.lang.management.ThreadMXBean                                                        
Warning: org.junit.internal.runners.statements.FailOnTimeout: can't find referenced class java.lang.management.ThreadMXBean                                                        
Warning: org.junit.rules.DisableOnDebug: can't find referenced class java.lang.management.ManagementFactory                                                                        
Warning: org.junit.rules.DisableOnDebug: can't find referenced class java.lang.management.RuntimeMXBean                                                                            
Warning: org.junit.rules.DisableOnDebug: can't find referenced class java.lang.management.ManagementFactory                                                                        
Warning: org.junit.rules.DisableOnDebug: can't find referenced class java.lang.management.RuntimeMXBean 

Which I think means that you can't add junit to the production implementation. If this is the case, it's probably fine to suppress because the GleanTest* classes are unlikely to be called from production code. However, if they are called, there may be a crash.

Dexterp37 commented 4 years ago

Which I think means that you can't add junit to the production implementation. If this is the case, it's probably fine to suppress because the GleanTest* classes are unlikely to be called from production code. However, if they are called, there may be a crash.

Yes, these classes won't ever be called from production. The Glean SDK throws if that's the case :)