Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up retrofit related long running tests (arrow-kt#2908) #2962

Merged

Conversation

bbuallbest
Copy link
Contributor

@bbuallbest bbuallbest commented Mar 7, 2023

Issue

This is a part of changes related to #2908 issue.

Description

Current PR raised to fix next long running tests connected with retrofit http client:

     should return IOError when no responsearrow.retrofit.adapter.either.networkhandling.NetworkEitherCallAdapterTestSuite:arrow-core-retrofit:test | PASSED | 10.889s
 (2) should return IOError when no responsearrow.retrofit.adapter.either.networkhandling.NetworkEitherCallAdapterTestSuite:arrow-core-retrofit:test | PASSED | 10.041s
 (1) should return IOError when no responsearrow.retrofit.adapter.either.networkhandling.NetworkEitherCallAdapterTestSuite:arrow-core-retrofit:test | PASSED | 9.448s

After changes I see next timings on my local machine:

    should return IOError when no response	0.237s	passed
(1) should return IOError when no response	0.204s	passed
(2) should return IOError when no response	0.203s	passed

Root cause

Out of the box retrofit is using default OkHttpClient if no other client passed during creation. Meanwhile OkHttpClient has default value of read timeout that is equal to 10 seconds.
As a fix - decreased read timeout to 0.2 seconds

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @bbuallbest for looking into this 🙌 🙏 And congrats on your first contribution!

@nomisRev nomisRev requested review from a team, franciscodr, i-walker and serras March 7, 2023 16:38
Copy link
Member

@serras serras 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 helping us with our testing :)

Copy link
Collaborator

@franciscodr franciscodr 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 your contribution, @bbuallbest!

@nomisRev nomisRev merged commit 4d53db7 into arrow-kt:main Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants