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.

Sign up for GitHub

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

Jump to bottom

Implement request timeouts in unittests #119

Merged
merged 2 commits into from
Aug 5, 2019
Merged

Implement request timeouts in unittests #119

merged 2 commits into from
Aug 5, 2019

Conversation

AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Jul 26, 2019 •
edited
Loading

Blocked by dependencies. The support code in the node is pretty simple though. And a unittest is included (which passes with all dependencies).

Requires:

The vibe.d support has not been implemented yet.

@AndrejMitrovic AndrejMitrovic added status-blocked Another issue/PR/external dependency is blocking completion of this PR/issue type-feature An addition to the system introducing new functionalities labels Jul 26, 2019
@AndrejMitrovic AndrejMitrovic removed the status-blocked Another issue/PR/external dependency is blocking completion of this PR/issue label Jul 29, 2019
@AndrejMitrovic
Copy link
Contributor Author

Unblocked.

@Geod24 Geod24 changed the title Implement request timeouts Implement request timeouts in unittests Jul 29, 2019
Geod24
Geod24 reviewed Jul 29, 2019
View reviewed changes
source/agora/test/NetworkClient.d Outdated
unittest
{
import vibe.core.log;
import core.thread;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How dare you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant the ordering. But actually the vibe import is a debug one, right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've always ordered them like this:

  • local imports first
  • dependencies second
  • core/std imports after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah the style guide says

Imports are organized by packages: agora first, dependencies second, std third, core last.

Ok, I will move the core import last.

@AndrejMitrovic
Copy link
Contributor Author

Ugh.. failures.

@AndrejMitrovic
Copy link
Contributor Author

Failure was unrelated, some package failed to download.

@AndrejMitrovic
Copy link
Contributor Author

It passes now.

@AndrejMitrovic
Copy link
Contributor Author

@Geod24 Geod24 changed the title Implement request timeouts Implement request timeouts in unittests yesterday

Sadness. Anyway, ready to merge?

@AndrejMitrovic
Copy link
Contributor Author

Ugh https://travis-ci.com/bpfkorea/agora/jobs/220751056

@AndrejMitrovic AndrejMitrovic force-pushed the request-timeouts branch 2 times, most recently from b6a9736 to 5ff42f4 Compare July 30, 2019 09:46
@AndrejMitrovic
Copy link
Contributor Author

AndrejMitrovic commented Jul 30, 2019 •
edited
Loading

Ah I think I know what's wrong. It's the logic in my code. I have a retry_delay + max_retries. So if the retry_delay is 100 msecs, and max_retries is 20, it should mean after 2 seconds the retrying should stop.

But it's not taking into account the time it takes for the request to time-out. So the actual time spent is:

retry_delay * max_retries * timeout

max_retries * (retry_delay + timeout)

I need to take this into account.. at least in my unittests.

@AndrejMitrovic AndrejMitrovic force-pushed the request-timeouts branch 3 times, most recently from f6a8cc0 to 2bf3373 Compare July 31, 2019 02:29
@AndrejMitrovic AndrejMitrovic mentioned this pull request Jul 31, 2019
Closed
@AndrejMitrovic AndrejMitrovic force-pushed the request-timeouts branch 3 times, most recently from 962c9e7 to fa389f0 Compare July 31, 2019 04:42
@AndrejMitrovic
Copy link
Contributor Author

It seems like Travis does some kind of dependency caching.. because it's not finding the localrest commit in my branch, even though it's there (I did update .gitmodules too).

@AndrejMitrovic AndrejMitrovic force-pushed the request-timeouts branch 3 times, most recently from 9bcf439 to ee4371d Compare August 1, 2019 00:59
@AndrejMitrovic
Copy link
Contributor Author

Ping.

@AndrejMitrovic
Copy link
Contributor Author

Re-ordered imports.

@AndrejMitrovic
Copy link
Contributor Author

Fixed.

@Geod24
Copy link
Collaborator

Geod24 commented Aug 1, 2019

Still red

@AndrejMitrovic
Copy link
Contributor Author

Great. Can we drop Mac support?

@AndrejMitrovic
Update LocalRest to latest v0.x.x
97cad03 
This adds support for request timeouts
@AndrejMitrovic
Implement request timeouts in unittests
3ef8b98 
This uses LocalRest's support for request timeouts,
but doesn't yet use vibe.d's request timeouts
as they haven't been implemented yet.
@Geod24 Geod24 merged commit e614d3d into bosagora:v0.x.x Aug 5, 2019
@AndrejMitrovic AndrejMitrovic deleted the request-timeouts branch August 5, 2019 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Geod24 Geod24 Geod24 approved these changes

Assignees
No one assigned
Labels
type-feature An addition to the system introducing new functionalities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@AndrejMitrovic @Geod24