Merge pull request #1186 from cloudbees-oss/v1-sync

Forward porting from v1

Author: kohsuke

smart_tests/commands/subset.py

@@ -400,7 +400,7 @@ def get_payload(self) -> dict[str, Any]:
 
     def _collect_potential_test_files(self):
         LOOSE_TEST_FILE_PATTERN = r'(\.(test|spec)\.|_test\.|Test\.|Spec\.|test/|tests/|__tests__/|src/test/)'
-        EXCLUDE_PATTERN = r'\.(xml|json|txt|yml|yaml|md)$'
+        EXCLUDE_PATTERN = r'(BUILD|Makefile|Dockerfile|LICENSE|.gitignore|.gitkeep|.keep|id_rsa|rsa|blank|taglib)|\.(xml|json|jsonl|txt|yml|yaml|toml|md|png|jpg|jpeg|gif|svg|sql|html|css|graphql|proto|gz|zip|rz|bzl|conf|config|snap|pem|crt|key|lock|jpi|hpi|jelly|properties|jar|ini|mod|sum|bmp|env|envrc|sh)$'  # noqa E501
 
         try:
             git_managed_files = subprocess.run(['git', 'ls-files'], stdout=subprocess.PIPE,

smart_tests/jar/exe_deploy.jar

@@ -1,14 +1,15 @@
 import json
-import os
 import sys
+import xml.etree.ElementTree as ET
+from os import path
 from pathlib import Path
 from typing import Annotated, Generator, List
 
 from junitparser import TestCase, TestSuite  # type: ignore
 
+import smart_tests.args4p.converters as converters
 import smart_tests.args4p.typer as typer
 
-from ..args4p.converters import path
 from ..commands.subset import Subset
 from ..testpath import TestPath
 from ..utils.logger import Logger
@@ -44,7 +45,7 @@ def record_tests(
         "--build-event-json",
         help="set file path generated by --build_event_json_file",
         multiple=True,
-        type=path(exists=True)
+        type=converters.path(exists=True)
     )] = None,
 ):
     """
@@ -64,15 +65,45 @@ def f(case: TestCase, suite: TestSuite, report_file: str) -> TestPath:
 
         # last path component is the target, the rest is package
         # TODO: does this work correctly when on Windows?
-        path = make_test_path(os.path.dirname(pkgNtarget), os.path.basename(pkgNtarget))
+        tp = make_test_path(path.dirname(pkgNtarget), path.basename(pkgNtarget))
 
         # let the normal path building kicks in
-        path.extend(default_path_builder(case, suite, report_file))
-        return path
+        tp.extend(default_path_builder(case, suite, report_file))
+        return tp
 
     client.path_builder = f
     client.check_timestamp = False
 
+    def parse_func(report: str) -> ET.ElementTree:
+        """
+        test result XML generated by Bazel's java_test rule (and possibly others) do not capture system-out/system-err.
+        The whole thing gets captured separately as test.log file in the same directory as test.xml.
+        This limits our ability to do useful things with data, so we go out of our way to capture it.
+        Ideally Bazel should do this. test.log captures the entire output from the whole session, and therefore
+        it is incapable of splitting log between different test classes.
+        """
+        tree = ET.parse(report)
+        root = tree.getroot()
+        for ts in root.findall('testsuite'):
+            # be defensive -- future/non-Java test rules might capture those
+            if ts.findtext('system-out', '') == "" and ts.findtext('system-err', '') == "":
+                logfile = path.join(path.dirname(report), 'test.log')
+                if path.exists(logfile):
+                    print("Found {}".format(logfile))
+                    for x in ts.findall('system-out'):
+                        ts.remove(x)
+                    with open(logfile, 'r', encoding='utf-8', errors='replace') as log_file:
+                        log = log_file.read()
+
+                    for c in ts.findall('testcase'):
+                        if c.findtext('system-out', '') == "" and c.findtext('system-err', '') == "":
+                            system_out = ET.SubElement(c, 'system-out')
+                            system_out.text = log
+
+        return tree  # type: ignore
+
+    client.junitxml_parse_func = parse_func
+
     if build_event_json_files:
         for l in parse_build_event_json(build_event_json_files):
             if l is None:

smart_tests/test_runners/bazel.py

@@ -89,6 +89,8 @@ public class CommitGraphCollector {
 
   private int maxDays;
 
+  private boolean reportAllFiles;
+
   private boolean audit;
 
   private boolean dryRun;
@@ -150,7 +152,7 @@ public void transfer(URL service, Authenticator authenticator, boolean enableTim
       }
       CloseableHttpResponse latestResponse = client.execute(new HttpGet(latestUrl.toExternalForm()));
       ImmutableList<ObjectId> advertised = getAdvertisedRefs(handleError(latestUrl, latestResponse));
-      honorMaxDaysHeader(latestResponse);
+      honorControlHeaders(latestResponse);
 
       // every time a new stream is needed, supply ByteArrayOutputStream, and when the data is all
       // written, turn around and ship that over
@@ -301,16 +303,21 @@ public void accept(VirtualFile f) {
     }
   }
 
-  /**
-   * When a user incorrectly configures shallow clone, the incremental nature of commit collection
-   * makes it really hard for us and users to collaboratively reset and repopulate the commit data.
-   * This server-side override mechanism makes it easier.
-   */
-  private void honorMaxDaysHeader(HttpResponse response) {
+  private void honorControlHeaders(HttpResponse response) {
+    // When a user incorrectly configures shallow clone, the incremental nature of commit collection
+    // makes it really hard for us and users to collaboratively reset and repopulate the commit data.
+    // This server-side override mechanism makes it easier.
     Header h = response.getFirstHeader("X-Max-Days");
     if (h!=null) {
       maxDays = Integer.parseInt(h.getValue());
     }
+    // File transfer is supposed to work incrementally, but we are investigating the problem where
+    // not all files get collected prior to commit collection, resulting in incomplete data on the server side.
+    // As a temporary mitigation, allow the server to request all files to be reported.
+    h = response.getFirstHeader("X-Report-All-Files");
+    if (h!=null) {
+      reportAllFiles = true;
+    }
   }
 
   private ImmutableList<ObjectId> getAdvertisedRefs(HttpResponse response) throws IOException {
@@ -427,16 +434,22 @@ public void transfer(Collection<ObjectId> advertised, Consumer<JSCommit> commitR
         // and it drastically cuts down the initial commit consumption of a new large repository.
         // ... except we do want to capture the head commit, as that makes it easier to spot integration problems
         // when `record build` and `record commit` are separated.
+
+        // two RevFilters are order sensitive. This is because CommitTimeRevFilter.after doesn't return false to
+        // filter things out, it throws StopWalkException to terminate the walk, never giving a chance for the other
+        // branch of OR to be evaluated. So we need to put ObjectRevFilter first.
         walk.setRevFilter(
             OrRevFilter.create(
-                CommitTimeRevFilter.after(System.currentTimeMillis() - TimeUnit.DAYS.toMillis(maxDays)),
-                new ObjectRevFilter(headId)));
+              new ObjectRevFilter(headId),
+              CommitTimeRevFilter.after(System.currentTimeMillis() - TimeUnit.DAYS.toMillis(maxDays))));
 
         for (ObjectId id : advertised) {
           try {
             RevCommit c = walk.parseCommit(id);
             walk.markUninteresting(c);
-            treeWalk.addTree(c.getTree());
+            if (!reportAllFiles) {
+              treeWalk.addTree(c.getTree());
+            }
           } catch (MissingObjectException e) {
             // it's possible that the server advertises a commit we don't have.
             //

src/main/java/com/launchableinc/ingest/commits/CommitGraphCollector.java

@@ -0,0 +1 @@
+Dummy standard output

tests/data/bazel/bazel-testlogs/src/test/java/com/ninjinkun/mylib_test1/test.log

@@ -1,7 +1,9 @@
 <?xml version='1.0' encoding='UTF-8'?>
 <testsuites>
-  <testsuite name='com.ninjinkun.MyLibTest1' timestamp='2020-12-16T03:11:43.526Z' hostname='localhost' tests='1' failures='0' errors='0' time='2.02' package='' id='0'>
+  <testsuite name='com.ninjinkun.MyLibTest1' timestamp='2020-12-16T03:11:43.526Z' hostname='localhost' tests='1' failures='1' errors='0' time='2.02' package='' id='0'>
     <properties />
-    <testcase name='testFibonacci' classname='com.ninjinkun.MyLibTest1' time='2.02' />
+    <testcase name='testFibonacci' classname='com.ninjinkun.MyLibTest1' time='2.02'>
+      <failure message='Yo!' type='java.lang.Exception'>java.lang.Exception: Yo!
+        at somewhere(Somewhere.java:33)</failure></testcase>
     <system-out />
     <system-err /></testsuite></testsuites>

tests/data/bazel/bazel-testlogs/src/test/java/com/ninjinkun/mylib_test1/test.xml

@@ -23,9 +23,9 @@
         { "type": "testcase", "name": "testFibonacci" }
       ],
       "duration": 2.02,
-      "status": 1,
-      "stdout": "",
-      "stderr": "",
+      "status": 0,
+      "stdout": "Dummy standard output\n",
+      "stderr": "java.lang.Exception: Yo!\n        at somewhere(Somewhere.java:33)",
       "data": null
     },
     {

tests/data/bazel/record_test_result.json

@@ -9,9 +9,9 @@
         { "type": "testcase", "name": "testFibonacci" }
       ],
       "duration": 2.02,
-      "status": 1,
-      "stdout": "",
-      "stderr": "",
+      "status": 0,
+      "stdout": "Dummy standard output\n",
+      "stderr": "java.lang.Exception: Yo!\n        at somewhere(Somewhere.java:33)",
       "data": null
     },
     {

tests/data/bazel/record_test_with_build_event_json_result.json

@@ -9,9 +9,9 @@
         { "type": "testcase", "name": "testFibonacci" }
       ],
       "duration": 2.02,
-      "status": 1,
-      "stdout": "",
-      "stderr": "",
+      "status": 0,
+      "stdout": "Dummy standard output\n",
+      "stderr": "java.lang.Exception: Yo!\n        at somewhere(Somewhere.java:33)",
       "data": null
     },
     {