On Thu, 19 Jun 2025 03:42:21 GMT, Manukumar V S <m...@openjdk.org> wrote:
>> Issue:
>> MyanmarTextTest.java produces a false positive result when some of the test
>> preconditions are not met. It checks whether certain fonts are present in
>> the system, for example, 'Padauk' on Linux. If the required font is missing,
>> the test simply returns early, and the test ends up passing, which is
>> incorrect. Ideally, it should throw a jtreg.SkippedException when the
>> necessary preconditions are not satisfied.
>>
>> Another scenario is that the test passes on headless machines even though it
>> creates GUI components. Ideally, when GUI components are created in code
>> running on a headless machine, a HeadlessException should be thrown.
>> However, since MyanmarTextTest.java exits before reaching the point where
>> the GUI is created (due to unmet preconditions), it incorrectly reports a
>> pass. This behavior may lead to a misinterpretation of the test as being
>> headless, which it is not.
>>
>> Fix:
>> Need to throw jtreg.SkippedException in cases where some pre-conditions for
>> running the test are not met.
>>
>> Testing:
>> Tested using mach5 in all available platforms and got full PASS.
>
> Manukumar V S has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Combined the fonts in to one list, formatting changes
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 56:
> 54: private static final String TEXT = "\u1000\u103C";
> 55:
> 56: private static final String OS_NAME =
> System.getProperty("os.name").toLowerCase();
The OS name isn't needed any longer.
test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 63:
> 61: "Myanmar Text",
> 62: "Noto Sans Myanmar");
> 63: private static final String FONT_NAME = selectFontName();
Suggestion:
"Noto Sans Myanmar");
private static final String FONT_NAME = selectFontName();
A blank line.
test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 76:
> 74: System.err.println("Checked fonts: " + fontList);
> 75: throw new SkippedException("Required fonts not installed for
> OS: "
> 76: + OS_NAME + ". Checked fonts: " + fontList);
Suggestion:
throw new SkippedException("No suitable font found out of the list:
"
+ String.join(", ", FONT_CANDIDATES));
I see no value in printing the same information twice, the exception message is
*always* printed, and it contains all the required details.
test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 145:
> 143: .filter(FONT_CANDIDATES::contains)
> 144: .findFirst()
> 145: .orElse(null);
I'm for preserving the original wrapping style where `.` align:
Suggestion:
return Arrays.stream(GraphicsEnvironment
.getLocalGraphicsEnvironment()
.getAvailableFontFamilyNames())
.filter(FONT_CANDIDATES::contains)
.findFirst()
.orElse(null);
-------------
PR Review: https://git.openjdk.org/jdk/pull/25879#pullrequestreview-2942345012
PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2156613372
PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2156614008
PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2156624016
PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2156627677
