gnodet-bot commented on code in PR #12695: URL: https://github.com/apache/maven/pull/12695#discussion_r4293030129
########## api/maven-api-core/src/main/java/org/apache/maven/api/build/report/BuildReport.java: ########## @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.api.build.report; + +import java.time.Duration; +import java.time.Instant; +import java.util.List; +import java.util.Objects; +import java.util.Optional; + +import org.apache.maven.api.BuildEnvironment; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Immutable; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.services.BuilderProblem; + +/** + * A structured report of a Maven build execution, persisted to + * {@code target/build-report.json} at the end of every build. + * <p> + * The report captures metadata, per-module results (including mojo execution + * timings), and any failures. It is intended to be consumed by tools, IDEs, + * CI systems, and LLM agents without having to re-run the build or parse + * console output. + * + * @since 4.1.0 + * @see ModuleReport + * @see FailureReport + */ +@Experimental +@Immutable +public interface BuildReport { + + /** + * Semantic version of the report format (e.g. {@code "1.0"}). + * <p> + * Versioning policy: + * <ul> + * <li><b>Minor</b> increments (1.0 → 1.1) are purely additive — + * new fields may appear, but existing fields keep their name, type, and + * meaning. Consumers that ignore unknown fields are forward-compatible.</li> + * <li><b>Major</b> increments (1.x → 2.0) signal a breaking change — + * fields may be removed, renamed, or change type. Consumers should check + * the major version and refuse to parse an unsupported major.</li> + * </ul> + * + * @return the semver report version string, never {@code null} + * @since 4.1.0 + */ + @Nonnull + String reportVersion(); Review Comment: **[Medium] `reportVersion()` and `formatVersion()` are two names for the same concept — pick one.** `reportVersion()` (String `"1.0"`, semver) and `formatVersion()` (int `1`, "schema version") both describe the format version of the build report. Their Javadocs are effectively identical in intent: one says "Semantic version of the report format", the other says "Schema version of the report format. Consumers should check this to handle forward compatibility." Having both on a public API that is `@Experimental` but already versioned creates confusion for consumers about which field to read, and they can diverge silently (`reportVersion = "2.0"`, `formatVersion = 1`). Options: 1. **Remove `formatVersion()`** — it predates proper semver and its `int` type cannot express minor/patch granularity. `reportVersion()` is strictly more expressive. Update all tests and JSON assertions from `"formatVersion": 1` → `"reportVersion": "1.0"`. 2. **Remove `reportVersion()`** — if semver is premature, keep `formatVersion` and bump it on breaking changes. Simpler but less expressive. 3. **Keep both but document the invariant** — if both must coexist (e.g. tooling needs the int for fast comparison), document the relationship explicitly: "major component of `reportVersion()` always equals `formatVersion()`" and enforce this in `DefaultBuildReport` with an assertion or a single source of truth: ```java private static final String REPORT_VERSION = "1.0"; private static final int FORMAT_VERSION = Integer.parseInt(REPORT_VERSION.split("\\.")[0]); ``` Option 1 (remove `formatVersion`) is the cleanest — the API is `@Experimental` and has not shipped yet. ########## impl/maven-core/src/test/java/org/apache/maven/internal/build/BuildReportJsonWriterTest.java: ########## @@ -0,0 +1,438 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.internal.build; + +import java.time.Duration; +import java.time.Instant; +import java.util.List; +import java.util.Map; + +import org.apache.maven.api.BuildEnvironment; +import org.apache.maven.api.build.report.BuildReport; +import org.apache.maven.api.build.report.BuildStatus; +import org.apache.maven.api.build.report.FailureReport; +import org.apache.maven.api.build.report.LogEvent; +import org.apache.maven.api.build.report.LogLevel; +import org.apache.maven.api.build.report.ModuleReport; +import org.apache.maven.api.build.report.MojoReport; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class BuildReportJsonWriterTest { + + private static final BuildEnvironment EMPTY_ENV = new DefaultBuildEnvironment( + List.of(), Map.of(), Map.of(), "", List.of(), List.of(), null, "FAIL_FAST", false, false, false, false, 1); + private static final Instant BASE_TIME = Instant.parse("2025-01-15T10:30:00Z"); + + @Test + void testSuccessfulBuildReport() { + List<LogEvent> mojoOutput = List.of( + new DefaultLogEvent( + BASE_TIME.plusSeconds(6), LogLevel.INFO, "Compiling 42 source files", "o.a.m.compiler", null), + new DefaultLogEvent(BASE_TIME.plusSeconds(7), LogLevel.INFO, "BUILD SUCCESS", "o.a.m.compiler", null)); + + MojoReport mojo = new DefaultMojoReport( + "org.apache.maven.plugins", + "maven-compiler-plugin", + "3.15.0", + "compile", + "default-compile", + "compile", + BuildStatus.SUCCESS, + BASE_TIME.plusSeconds(5), + Duration.ofMillis(2100), + mojoOutput); + + List<LogEvent> moduleOutput = List.of(new DefaultLogEvent( + BASE_TIME.plusSeconds(2), + LogLevel.INFO, + "Resolving dependencies for maven-core", + "o.a.m.resolver", + null)); + + ModuleReport module = new DefaultModuleReport( + "org.apache.maven", + "maven-core", + "4.1.0-SNAPSHOT", + BuildStatus.SUCCESS, + BASE_TIME.plusSeconds(1), + Duration.ofMillis(12345), + List.of(mojo), + moduleOutput); + + List<LogEvent> buildOutput = List.of( + new DefaultLogEvent(BASE_TIME, LogLevel.INFO, "Reactor Build Order:", "o.a.m.reactor", null), + new DefaultLogEvent(BASE_TIME, LogLevel.INFO, "Maven Core", "o.a.m.reactor", null)); + + BuildReport report = new DefaultBuildReport( + EMPTY_ENV, + BuildStatus.SUCCESS, + Duration.ofMillis(30000), + BASE_TIME, + "4.1.0-SNAPSHOT", + "21.0.1", + List.of("clean", "install"), + "org.apache.maven:maven:4.1.0-SNAPSHOT", + true, + 4, + List.of(module), + List.of(), + List.of(), + buildOutput); + + String json = BuildReportJsonWriter.toJson(report); + + assertTrue(json.contains("\"formatVersion\": 1")); Review Comment: **[Low] Test does not assert the new `reportVersion` field in the JSON output.** `reportVersion` is now the first field written by `writeReport()` (added before `formatVersion`), but `testSuccessfulBuildReport()` only asserts `"formatVersion": 1`. Add: ```suggestion assertTrue(json.contains("\"reportVersion\": \"1.0\"")); assertTrue(json.contains("\"formatVersion\": 1")); ``` Same gap exists in `BuildReportCollectorTest` (line 203: `assertTrue(content.contains("\"formatVersion\": 1"))`) — add a matching assertion for `reportVersion` there too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
