On Wed, 18 Jun 2025 19:18:06 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: > > Added blank line before OS_NAME Changes requested by aivanov (Reviewer). test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 59: > 57: private static final String[] FONTS_WINDOWS = {"Myanmar Text", "Noto > Sans Myanmar"}; > 58: private static final String[] FONTS_LINUX = {"Padauk", "Noto Sans > Myanmar", "Myanmar Text"}; > 59: private static final String[] FONTS_MACOS = {"Myanmar MN", "Noto Sans > Myanmar", "Myanmar Text"}; I'm unsure if Noto Sans fonts are installed by default on Windows or macOS. Similarly, "Myanmar Text" is unlikely to be installed on Linux. Since there's a list of possible fonts, the list could well be *platform-independent* — just look for a first match. test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 61: > 59: private static final String[] FONTS_MACOS = {"Myanmar MN", "Noto Sans > Myanmar", "Myanmar Text"}; > 60: > 61: private static final String[] FONT_CANDIDATES = > selectFontCandidates(); Suggestion: private static final List<String> FONT_CANDIDATES = List.of("Myanmar MN", "Padauk", "Myanmar Text", "Noto Sans Myanmar"); test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 74: > 72: throw new SkippedException("Unsupported OS: " + OS_NAME); > 73: } > 74: if (FONT_NAME == null) { With the changes that I propose, `FONT_NAME = selectFontName()`, this will be the only condition: if the font name is `null`, no suitable font is found — skip the test. test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 105: > 103: main.setLayout(new BoxLayout(main, BoxLayout.Y_AXIS)); > 104: main.add(myanmarTF); > 105: main.setBorder(BorderFactory.createEmptyBorder(7, 7, 7, 7)); I see no reason for removing this blank line. test/jdk/java/awt/font/TextLayout/MyanmarTextTest.java line 153: > 151: } > 152: > 153: private static String selectAvailableFont() { private static String selectFontName() { return Arrays.stream(GraphicsEnvironment .getLocalGraphicsEnvironment() .getAvailableFontFamilyNames()) .filter(FONT_CANDIDATES::contains) .findFirst() .orElse(null); } ------------- PR Review: https://git.openjdk.org/jdk/pull/25879#pullrequestreview-2940458706 PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2155365237 PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2155390963 PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2155402010 PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2155366028 PR Review Comment: https://git.openjdk.org/jdk/pull/25879#discussion_r2155392324