Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

fix: ConnectivityChecker should return false if offline and no permission #420

Merged
merged 18 commits into from
May 20, 2020
Merged

fix: ConnectivityChecker should return false if offline and no permission #420

merged 18 commits into from
May 20, 2020

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented May 18, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

fix: ConnectivityChecker should return false if offline and no permission

💡 Motivation and Context

ConnectivityChecker was assuming connected even if offline (because of permissions)

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing

🔮 Next steps

@marandaneto marandaneto requested a review from bruno-garcia as a code owner May 18, 2020 14:13
@marandaneto marandaneto changed the title fix/connectivity checker fix: ConnectivityChecker should return false if offline and no permission May 18, 2020
@codecov-io
Copy link

codecov-io commented May 18, 2020

Codecov Report

Merging #420 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #420   +/-   ##
=========================================
  Coverage     60.18%   60.18%           
  Complexity      809      809           
=========================================
  Files            92       92           
  Lines          3747     3747           
  Branches        360      360           
=========================================
  Hits           2255     2255           
  Misses         1338     1338           
  Partials        154      154           

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 51cd51e...882661f. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

I like the naming of the test methods a lot. Most of the code looks really good. Still I made many comments to maybe improve many parts.

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #420 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #420      +/-   ##
============================================
- Coverage     60.19%   60.04%   -0.15%     
- Complexity      809      813       +4     
============================================
  Files            92       93       +1     
  Lines          3748     3742       -6     
  Branches        360      363       +3     
============================================
- Hits           2256     2247       -9     
- Misses         1338     1339       +1     
- Partials        154      156       +2     
Impacted Files Coverage Δ Complexity Δ
...java/io/sentry/core/transport/AsyncConnection.java 68.50% <0.00%> (-2.42%) 19.00% <0.00%> (ø%)
...e/src/main/java/io/sentry/core/EnvelopeSender.java 66.31% <0.00%> (-0.70%) 16.00% <0.00%> (ø%)
...re/src/main/java/io/sentry/core/SentryOptions.java 80.53% <0.00%> (ø) 73.00% <0.00%> (ø%)
...c/main/java/io/sentry/core/cache/SessionCache.java 53.63% <0.00%> (ø) 23.00% <0.00%> (ø%)
...try/core/transport/RetryingThreadPoolExecutor.java
...entry/core/transport/QueuedThreadPoolExecutor.java 78.94% <0.00%> (ø) 7.00% <0.00%> (?%)
...re/src/main/java/io/sentry/core/util/LogUtils.java 70.00% <0.00%> (ø) 4.00% <0.00%> (?%)
.../src/main/java/io/sentry/core/SendCachedEvent.java 55.76% <0.00%> (+0.86%) 8.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 0363118...d17f542. Read the comment docs.

@philipphofmann
Copy link
Member

I opened up a PR on your repo to make use of beforeTest in ConnectivityCheckerTest. I prefer it because it moves the whole mocking setup to beforeTest and the test methods only contain what you are testing. If you like it merge it if not just close my PR.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for including my feedback. Looks way better IMO 👍

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Awesome job for including lots of my feedback. I really like this PR now.

@philipphofmann
Copy link
Member

65 comments 😄

@marandaneto marandaneto merged commit 8ce5093 into getsentry:master May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants