Rework 5s sleep with 100ms intervals

[rutgerva]

Reworked Test case to check every 100ms and break out the loop once a response has been written to the log file.

Fixes #231

[basil]

Of course the best solution would be to find some method that blocks until the log handlers have executed and flushed their output, but this is certainly a more efficient workaround than the hasty five-second sleep I added the other day.

[timja]

FWIW on linux the test took less than 1s and on windows around 2s
https://ci.jenkins.io/blue/organizations/jenkins/Core%2Fwinstone/detail/PR-233/1/tests

[olamy]

the idea of a loop should be to wait a certain time until reaching a conditon (it;s a fragile 5s timeout with such loop)
if you don't want to do it manually this can be easily achieved using http://www.awaitility.org/

[basil]

Well, if we are going to be expounding our opinions about how to solve this problem in the most perfect way possible, which is what we seem to be doing, then I think the best solution would be to block until the log handler is flushed. That would eliminate the need for any fragile busy-waiting in the first place.

You can see my sketch in #219 for an example of how to do synchronization between JUL and application code. While it is a bit tricky to understand at first, once you spend a few minutes stepping through JUL in a debugger and understand the basic code flow it all starts to make sense.

Of course, I already approved this change, and I personally feel the current PR is good enough, but if others feel that blocking this PR is a productive way to engage with a first-time contributor, far be it from me to stand in the way of that.

I believe the above post was polite, technical, and community-oriented.

[olamy]

Well, if we are going to be expounding our opinions about how to solve this problem in the most perfect way possible, which is what we seem to be doing, then I think the best solution would be to block until the log handler is flushed. That would eliminate the need for any fragile busy-waiting in the first place.

in this case we will rely on default surefire timeout which is very long.
Not sure we want to wait so long in case of a code change which breaks this test.

We can eventually move the timeout to a higher value (10s) and assume if the content has not been written to the file there is a regression

You can see my sketch in #219 for an example of how to do synchronization between JUL and application code. While it is a bit tricky to understand at first, once you spend a few minutes stepping through JUL in a debugger and understand the basic code flow it all starts to make sense.

Of course, I already approved this change, and I personally feel the current PR is good enough, but if others feel that blocking this PR is a productive way to engage with a first-time contributor, far be it from me to stand in the way of that.

I believe the above post was polite, technical, and community-oriented.

I guess when proposing a PR. The contributor is asking review on how to eventually improve his change.
So I just made a comment saying from a sleep of 5s we go to a loop with non deterministic time.
I only suggest to change to something more simple (less code to maintain and using a library which is well known and famous for such usage).

The check could be simply reduce to:

await().atMost(5, TimeUnit.SECONDS).until(()-> FileUtils.readFileToString(logFile, StandardCharsets.UTF_8),
                containsString(String.format("127.0.0.1 - - GET /examples/CountRequestsServlet HTTP/1.1 200%n")));

and obviously adding the dependency pom:

    <dependency>
      <groupId>org.awaitility</groupId>
      <artifactId>awaitility</artifactId>
      <version>4.2.0</version>
      <scope>test</scope>
    </dependency>

The goal of my comment is to propose an improvement which should be the goal of any PR's comment.
I don't understand why this is non productive?
Maybe the contributor didn't know this framework and will learn something new which will improve his skills.
That's how I imagine contributing: improve a software you like, eventually learn something new and participate in the community life.

[basil]

in this case we will rely on default surefire timeout which is very long.

Not necessarily: Object#wait can take a timeout if so desired. With the use of one of the Object#wait methods that takes a timeout, my ideal solution is strictly better than any solution based on busy-waiting, because in the common case (the success case) the test thread could be notified immediately when the condition variable changes, without having to perform the first sleep interval at all.

I only suggest to change to something more simple (less code to maintain and using a library which is well known and famous for such usage).

Introducing a new library also has its own maintenance costs, though I am OK with Awaitility.

I guess when proposing a PR. The contributor is asking review on how to eventually improve his change.
The goal of my comment is to propose an improvement which should be the goal of any PR's comment.
I don't understand why this is non productive?

You and I must have different definitions of "review" and "improvement". When your first reaction to a pull request is to hit the big red "Request Changes" button without providing the reasoning (as you did in jenkinsci/pom#209 (review)), this can come across as heavy-handed. I wonder how the author of this PR would feel if I hit the big red "Request Changes" button and insisted on implementing the ideal solution of a custom log handler with perfect synchronization. Sure, it is strictly more efficient than busy-waiting. But it is also significantly more implementation effort for a negligible gain. If I were to do that, I might very well be called unreasonable. But suggesting it as optional feedback could very well be reasonable, since it leaves the choice up to the author. My personal experience has been that you tend to deny me the freedom to make my own judgments, for example regarding when it is safe to delete dead code. I used to work on OS kernel modules, so I know that some situations require complete and total rigor when it comes to correctness and synchronization. This minor test case is not one of them, in my opinion.

As another contributor observed in jenkinsci/pipeline-stage-view-plugin#122 (comment), "your approach is very much in the minority of maintainers." But I respect that you have a different approach. I only hope that other contributors to this repository will not feel as discouraged as I have been by your code reviews.

[olamy]

If you have any personal grief/attack to me please send me an email.
But such comments has nothing to do here.
anyway I'm answering to such personal attach: I guess I retracted my comment. (jenkinsci/pom#209 (review))
regarding the second one: I have a very long history in opensource and rewarding contributors has always been my first consideration. I used to say thanks on Jira with attached patch file. But now with gh PRs is a different.
Do you add a "thank you" to every single PR you merge?
anyway my comment was including a "it was perfect" to congratulate the contributor.
I just made a bit of humour see the smiley but he didn't get it. well we do not have all the same approach regarding humour.
cheers have a good week end

[olamy]

anyway feel free to do whatever you want here.
this has turn into some personal attack to myself and I'm interested to participate anymore.

[olamy]

no comment

[basil]

Like you, I would much rather focus on technical topics and getting code merged.

the idea of a loop should be to wait a certain time until reaching a condition

Which is what the break statement in this submission is for - to stop waiting once the condition (the file being non-empty) is reached, or the timeout expires: whichever comes first.

it;s a fragile 5s timeout with such loop

I do not see what is fragile about the code the author has submitted. It seems perfectly correct to me. Sure, a few lines could be shaved off by introducing a new dependency on Awaitility, but I see no reason why that is significant enough to request changes. That seems like a matter of personal preference to me.

If there is something incorrect about this submission that merits a blocking review, I am not seeing it.

[basil]

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Thanks!

[basil]

Thanks for the PR!