Skip to content

Add timeout with Duration and apply larger timeouts to tests that oft… - #115

Merged
gnodet merged 16 commits into
apache:masterfrom
gnodet:fix-ci
Feb 28, 2020
Merged

Add timeout with Duration and apply larger timeouts to tests that oft…#115
gnodet merged 16 commits into
apache:masterfrom
gnodet:fix-ci

Conversation

@gnodet

@gnodet gnodet commented Feb 27, 2020

Copy link
Copy Markdown
Contributor

…en fail on windows

@gnodet

gnodet commented Feb 27, 2020

Copy link
Copy Markdown
Contributor Author

I've merged some changes to the CI, but we still have tests that often fail. The goal is to raise the timeout when possible to see if the CI is more green.

@lgoldstein lgoldstein left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the idea - see 2-3 minor comments

Comment thread sshd-common/src/main/java/org/apache/sshd/common/future/VerifiableFuture.java Outdated
Comment thread sshd-common/src/main/java/org/apache/sshd/common/future/WaitableFuture.java Outdated
Comment thread sshd-common/src/main/java/org/apache/sshd/common/future/WaitableFuture.java Outdated
@gnodet
gnodet force-pushed the fix-ci branch 2 times, most recently from 7f9a7cd to c9d2ebe Compare February 27, 2020 14:45
@lgoldstein

Copy link
Copy Markdown
Contributor

Looks fine

@lgoldstein

Copy link
Copy Markdown
Contributor

The goal is to raise the timeout when possible to see if the CI is more green.

Let's be careful not to raise it too much or failing tests will slow the tests as a whole - if each test is allowed ~30 sec. to fail and we have 100's of tests we might get a total build time of hours.

@gnodet

gnodet commented Feb 27, 2020 via email

Copy link
Copy Markdown
Contributor Author

@lgoldstein

Copy link
Copy Markdown
Contributor

My concern is that while right now tests are failing fast, in the future there might be some new code we add that causes many tests to fail after waiting a long time - thus increasing the build time even on our laptops significantly from a few minutes to many minutes. This would have a negative impact on the development cycles - and we are both pressed for time since we are doing this in our spare time. Little as it is I would rather spend my time fixing bugs rather than fixing, waiting 40 minutes for the relevant tests to fail and thus signal that I have to go and re-examine the fix...

1 similar comment
@lgoldstein

Copy link
Copy Markdown
Contributor

My concern is that while right now tests are failing fast, in the future there might be some new code we add that causes many tests to fail after waiting a long time - thus increasing the build time even on our laptops significantly from a few minutes to many minutes. This would have a negative impact on the development cycles - and we are both pressed for time since we are doing this in our spare time. Little as it is I would rather spend my time fixing bugs rather than fixing, waiting 40 minutes for the relevant tests to fail and thus signal that I have to go and re-examine the fix...

@gnodet

gnodet commented Feb 27, 2020 via email

Copy link
Copy Markdown
Contributor Author

@lgoldstein

lgoldstein commented Feb 27, 2020

Copy link
Copy Markdown
Contributor

That would be ideal... if possible, the the relevant timeouts in the tests can simply be written as

private static final Duration TIMEOUT = Duration.ofSeconds(System.getIntProperty("property.controlled.by.profile", ...some default value...)); // or something to this effect.

@lgoldstein

lgoldstein commented Feb 27, 2020

Copy link
Copy Markdown
Contributor

I thought of something extra: if indeed we can use a Maven profile then instead of an absolute number for the timeouts we can use a "factor":

 // or something to this effect.
private static final Duration TIMEOUT =
    Duration.ofSeconds(
        (int) (...default value... * System.getFloatProperty("property.controlled.by.profile", 1.0f))));

or both (using 2 separate properties) - as the case may be: whether we need an absolute of a relative number

@gnodet
gnodet merged commit ee3671c into apache:master Feb 28, 2020
@gnodet
gnodet deleted the fix-ci branch February 28, 2020 20:30
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.

2 participants