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

add a DateTimeProvider for making retry after testable #391

Merged
merged 3 commits into from
May 6, 2020
Merged

add a DateTimeProvider for making retry after testable #391

merged 3 commits into from
May 6, 2020

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented May 6, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

add a DateTimeProvider for making retry after testable

💡 Motivation and Context

we were using Thread.sleep(x) and this was slowing down the tests and could also become flakey, with this we can assert things properly.
thanks @philipphofmann for the code review #381 good catch.

💚 How did you test it?

unit tests.

📝 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 6, 2020 12:45
@marandaneto marandaneto requested a review from philipphofmann May 6, 2020 12:48
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.

Nice that we have a CurrentDateProvider now. I added a few suggestions to hopefully improve this a little bit.

@@ -32,6 +33,7 @@ class HttpTransportTest {
var readTimeout = 500
var bypassSecurity = false
val connection = mock<HttpURLConnection>()
val currentDateProvider = mock<ICurrentDateProvider>()
Copy link
Member

Choose a reason for hiding this comment

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

Instead of mocking this all the time, we could implement a TestCurrentDateProvider that always returns the same time like new Date(0) and use it here. Makes the code cleaner IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we'd need to have more than 1 fake though, as tests would need to return also values different than Date(0) eg if isRetryAfter returns false when the time has passed.
I think it's not a good idea and also not sure how to test this use case if the return is always the same?

what's your LOGAF here?

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@88e8dbc). Click here to learn what that means.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #391   +/-   ##
=========================================
  Coverage          ?   59.75%           
  Complexity        ?      788           
=========================================
  Files             ?       89           
  Lines             ?     3620           
  Branches          ?      350           
=========================================
  Hits              ?     2163           
  Misses            ?     1305           
  Partials          ?      152           
Impacted Files Coverage Δ Complexity Δ
.../io/sentry/core/transport/CurrentDateProvider.java 33.33% <33.33%> (ø) 1.00 <1.00> (?)
...n/java/io/sentry/core/transport/HttpTransport.java 73.60% <100.00%> (ø) 31.00 <1.00> (?)
sentry-core/src/main/java/io/sentry/core/Dsn.java 94.11% <0.00%> (ø) 12.00% <0.00%> (?%)
...ore/src/main/java/io/sentry/core/protocol/Gpu.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...c/main/java/io/sentry/core/protocol/Mechanism.java 37.93% <0.00%> (ø) 6.00% <0.00%> (?%)
...c/main/java/io/sentry/core/SentryEnvelopeItem.java 80.48% <0.00%> (ø) 10.00% <0.00%> (?%)
.../src/main/java/io/sentry/core/SystemOutLogger.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...src/main/java/io/sentry/core/protocol/Browser.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...core/src/main/java/io/sentry/core/SentryLevel.java 100.00% <0.00%> (ø) 1.00% <0.00%> (?%)
...in/java/io/sentry/core/protocol/SentryPackage.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
... and 81 more

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 88e8dbc...0ba6990. Read the comment docs.

@marandaneto marandaneto requested a review from philipphofmann May 6, 2020 14:43
@marandaneto marandaneto merged commit 92c416e into getsentry:master May 6, 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