RFR: 7903750: TimeBudget (-tb) does not fulfill its promisses [v3]

[Jiří Vaněk]

On Fri, 21 Mar 2025 10:29:08 GMT, Galder Zamarreño <galder at openjdk.org> wrote:

>> Jiří Vaněk has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Used  WARNING: and FATAL: instead of +++ WARNING - and +++ FATAL -
>>  - renamed getNiceMsTimeDate to formatMsToDaysAndTime
>
> jcstress-core/src/main/java/org/openjdk/jcstress/TimeBudget.java line 152:
> 
>> 150:         boolean print=false;
>> 151:         if (expectedPerTest > budget.milliseconds() * 2l) {
>> 152:             System.out.println(" + +++ FATAL - your tests will never finish as expected. They will run much longer ");
> 
> I don't think you should assume `System.out`. All these prints should be on a given `PrintStream out`, see `printOn()` method.
> 
> The formatting of the message does not seem to adhere to other messages. You should see how messages are printed elsewhere in the code and try to use same format? E.g.
> 
> 
> out.println("FATAL: No matching tests.")
> 
> 
> Also, about this particular message, if this is fatal jcstress should not run. I don't think this is really happening here. The `print=true` does not cause jcstress not to run.

Damn, sorry for this.  This was changed globa-wide recently, and I had kept it correctly in all prs. Ibviously I had become less vigilant at the end, sorry. Fixed here and also in one  more PR which was affected.

> jcstress-core/src/main/java/org/openjdk/jcstress/TimeBudget.java line 160:
> 
>> 158:         }
>> 159:         if (print) {
>> 160:             System.out.println(" | For " + expectedTests + " with concurrency factor of " + getConcurentTestsFactor()
> 
> If it was me, I would switch these `println` calls for `printf` to make the messages more readable.

I had rewrote it to printf, but at the end found the println slightly more readable. It seems println is a bit more used then printf in the prject, so unless yuo insists I will keep println.

> jcstress-core/src/main/java/org/openjdk/jcstress/infra/grading/ReportUtils.java line 310:
> 
>> 308:     }
>> 309: 
>> 310:     public static String getNiceMsTimeDate(long ms) {
> 
> I would not use the word `Nice`, what does it mean here? Something like `formatTimeDate` or `showTimeDate` would be fine for me.

Renamed to formatMsToDaysAndTime, wdyt?

-------------

PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007512874
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007515070
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007510865