Support gradle 9.4.0 test html in reports

[peterzhuamazon]

Description

Support gradle 9.4.0 test html in reports

Issues Resolved

Resolves #6057

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 1f2fe65)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Support Gradle 9.4.0 new "Failed" section HTML format for search-relevance Relevant files: src/report_workflow/test_report_runner.py tests/tests_report_workflow/test_test_report_runner.py tests/tests_report_workflow/data/test-results/123/integ-test/search-relevance/with-security/opensearch-integ-test/index.html tests/tests_report_workflow/data/test-results/123/integ-test/search-relevance/without-security/opensearch-integ-test/index.html tests/tests_report_workflow/data/test-results/123/integ-test/search-relevance/with-security/search-relevance.yml tests/tests_report_workflow/data/test-results/123/integ-test/search-relevance/without-security/search-relevance.yml Sub-PR theme: Handle FAIL status with no failed tests section in HTML reports (k-NN case) Relevant files: src/report_workflow/test_report_runner.py tests/tests_report_workflow/test_test_report_runner.py tests/tests_report_workflow/data/test-results/123/integ-test/k-NN/with-security/opensearch-integ-test/index.html tests/tests_report_workflow/data/test-results/123/integ-test/k-NN/without-security/opensearch-integ-test/index.html tests/tests_report_workflow/data/test-results/123/integ-test/k-NN/with-security/k-NN.yml tests/tests_report_workflow/data/test-results/123/integ-test/k-NN/without-security/k-NN.yml

⚡ Recommended focus areas for review Incomplete Condition The new elif failed_header branch searches for <h2>Failed</h2> and then looks for a parent <div class="tab">. However, the condition if path_td and failures_tds only checks that both elements exist but does not verify that failures_tds actually contains a non-zero failure count. This could lead to rows with 0 failures being incorrectly added to failed_test_list. elif failed_header: # https://ci.opensearch.org/ci/dbc/integ-test/3.6.0/11802/linux/x64/tar/test-results/10988/integ-test/search-relevance/without-security/opensearch-integ-test/index.html failed_div = failed_header.find_parent("div", class_="tab") if failed_div: for row in failed_div.find_all("tr"): path_td = row.find("td", class_="path") failures_tds = row.find_all("td", class_="failures") if path_td and failures_tds: failed_test_list.append(path_td.get_text(strip=True)) else: logging.info(f"Test Result File Has Changed Format {result_path}") failed_test_list.append("Test Result File Has Changed Format") Filter Specificity The filter for opensearch HTML result files was changed from f.endswith("index.html") to f.endswith("opensearch-integ-test/index.html"). This is more specific, but it may silently exclude valid result files if the directory structure differs (e.g., different task names or nested paths). Consider whether this assumption about the path structure is always valid. test_result_files = [f for f in result_files if f.endswith("opensearch-integ-test/index.html")] Ambiguous Fallback The elif soup.find("div", class_="summaryGroup") branch appends "Test FAIL But Test Result File Has No Failed Test" when the test result is FAIL but no failed tests section is found. This could be misleading in reports if the HTML format changes again in the future, as it silently swallows unrecognized formats without distinguishing them from the "no failures section" case. elif soup.find("div", class_="summaryGroup"): # https://ci.opensearch.org/ci/dbc/integ-test/3.6.0/11802/linux/x64/tar/test-results/10988/integ-test/k-NN/with-security/opensearch-integ-test/index.html logging.info(f"Test FAIL But Test Result File Has No Failed Test {result_path}") failed_test_list.append("Test FAIL But Test Result File Has No Failed Test")

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 1f2fe65
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Verify failure count before adding to failed list The condition if path_td and failures_tds only checks that failures_tds is non-empty, but doesn't verify that the failure count is actually greater than zero. A row with class_="failures" on the td element may still represent a test with 0 failures in some edge cases. You should also verify the failures count value is non-zero to avoid false positives. src/report_workflow/test_report_runner.py [328-334] failed_div = failed_header.find_parent("div", class_="tab") if failed_div: for row in failed_div.find_all("tr"): path_td = row.find("td", class_="path") failures_tds = row.find_all("td", class_="failures") if path_td and failures_tds: + # Only add if there's at least one failures td that isn't the success rate cell failed_test_list.append(path_td.get_text(strip=True)) Suggestion importance[1-10]: 2 __ Why: The suggestion points out a potential edge case but the improved_code is essentially identical to the existing_code (only adds a comment), making it not a real fix. The HTML structure in the test data shows class_="failures" is only applied to rows with actual failures, so the concern is largely theoretical.	Low
	Clarify ambiguous fallback branch for missing failures section This branch is triggered when summaryGroup exists but neither target_header ("Failed tests") nor failed_header ("Failed") is found. However, this could also match pages where tests actually passed (status is "PASS") but the caller invoked get_failed_tests anyway. The logic should be consistent with the caller's intent, but more importantly, the k-NN test HTML files in the test data show 0 failures yet the test asserts status == "FAIL" — this means the YAML test_result: FAIL drives the status, not the HTML content. This branch will be hit for any page with a summaryGroup div that has no "Failed" header, including pages with 0 failures, potentially producing misleading error messages. src/report_workflow/test_report_runner.py [338-341] elif soup.find("div", class_="summaryGroup"): # https://ci.opensearch.org/ci/dbc/integ-test/3.6.0/11802/linux/x64/tar/test-results/10988/integ-test/k-NN/with-security/opensearch-integ-test/index.html - logging.info(f"Test FAIL But Test Result File Has No Failed Test {result_path}") + # No "Failed" section found in the report despite test_result being FAIL + logging.info(f"Test FAIL But Test Result File Has No Failed Tests Section {result_path}") failed_test_list.append("Test FAIL But Test Result File Has No Failed Test") Suggestion importance[1-10]: 1 __ Why: The suggestion only changes a log message string (not the appended string), and the improved_code is nearly identical to existing_code. The behavioral concern raised is valid but not addressed by the proposed change.	Low

---

Previous suggestions

Suggestions up to commit 26ab68a

Category	Suggestion	Impact
Possible issue	Filter rows with actual failures only The condition if path_td and failures_tds only checks that failures_tds is non-empty, but it doesn't verify that the row actually has a non-zero failure count. This could cause rows with 0 failures (but having a failures class for styling) to be incorrectly added to the failed test list. You should also verify the failure count is greater than zero by checking the text content of the failures cell. src/report_workflow/test_report_runner.py [328-334] failed_div = failed_header.find_parent("div", class_="tab") if failed_div: for row in failed_div.find_all("tr"): path_td = row.find("td", class_="path") failures_tds = row.find_all("td", class_="failures") if path_td and failures_tds: - failed_test_list.append(path_td.get_text(strip=True)) + try: + failure_count = int(failures_tds[0].get_text(strip=True).replace("%", "").strip()) + if failure_count > 0: + failed_test_list.append(path_td.get_text(strip=True)) + except ValueError: + failed_test_list.append(path_td.get_text(strip=True)) Suggestion importance[1-10]: 5 __ Why: Looking at the HTML test data, the class_="failures" is only applied to rows that actually have failures (e.g., <td class="failures">50%</td>), so the current code correctly identifies failing rows. However, the suggestion has merit as a defensive check since the failures class could theoretically appear on rows with 0 failures in edge cases. The improvement is minor since the existing HTML structure already ensures only failed rows have this class.	Low
General	Avoid overly restrictive file path filtering The filter now only matches files ending with opensearch-integ-test/index.html, which is more restrictive than the previous index.html check. This could miss valid result files that use a different directory name convention. Consider whether this constraint is intentional and if it could exclude legitimate test result files from other components. src/report_workflow/test_report_runner.py [136-137] if self.name == "opensearch": - test_result_files = [f for f in result_files if f.endswith("opensearch-integ-test/index.html")] + test_result_files = [f for f in result_files if f.endswith("/index.html")] Suggestion importance[1-10]: 4 __ Why: The PR intentionally changed the filter from index.html to opensearch-integ-test/index.html to be more specific, as seen in the comment referencing CI URLs. The suggestion to revert to /index.html would be less precise and could include unintended files. The current restriction appears deliberate based on the PR context.	Low

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 1f2fe65.

Path	Line	Severity	Description
tests/tests_report_workflow/data/dist/opensearch/manifest.yml	42	low	Placeholder values 'abc123' used for both ref and commit_id fields of the search-relevance component. While this is test data, using non-verifiable commit references could mask supply chain concerns in production manifests. No evidence of malicious intent — appears to be a test fixture shortcut.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 1f2fe65

[codecov]

Codecov Report

❌ Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.60%. Comparing base (4a728f9) to head (1f2fe65).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/report_workflow/test_report_runner.py	88.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6065      +/-   ##
==========================================
- Coverage   96.61%   96.60%   -0.01%     
==========================================
  Files         405      405              
  Lines       18851    18895      +44     
==========================================
+ Hits        18212    18254      +42     
- Misses        639      641       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.