Skip to content

Commit

Permalink
Use pretty validation output with smithy diff
Browse files Browse the repository at this point in the history
Smithy diff now uses the pretty validation output.

The old and new models are both validated, but only build-failing
errors are shown. When the original model fails, a label is added
before ERROR/DANGER to show it's on the OLD model. When the new
model fails, it shows a NEW label.

When running the diff with two valid models, events have a label
of DIFF. The events shown here use the --severity option and default
to WARNING when not set.
  • Loading branch information
mtdowling committed Apr 2, 2023
1 parent 670e5d9 commit dcd7a75
Show file tree
Hide file tree
Showing 6 changed files with 245 additions and 77 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file 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 software.amazon.smithy.cli;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;

import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.UncheckedIOException;
import java.nio.file.Path;
import org.junit.jupiter.api.Test;
import software.amazon.smithy.utils.ListUtils;

public class DiffCommandTest {
@Test
public void passingDiffEventsExitZero() {
IntegUtils.withTempDir("diff", dir -> {
Path a = dir.resolve("a.smithy");
writeFile(a, "$version: \"2.0\"\nnamespace example\nstring A\n");

RunResult result = IntegUtils.run(dir, ListUtils.of("diff", "--old", a.toString(), "--new", a.toString()));
assertThat(result.getExitCode(), equalTo(0));
});
}

@Test
public void showsLabelForOldModelEvents() {
IntegUtils.withTempDir("diff", dir -> {
Path a = dir.resolve("a.smithy");
writeFile(a, "$version: \"2.0\"\nnamespace example\n@aaaaaa\nstring A\n");

RunResult result = IntegUtils.run(dir, ListUtils.of("diff", "--old", a.toString(), "--new", a.toString()));
assertThat(result.getExitCode(), equalTo(1));
assertThat(result.getOutput(), containsString("── OLD ─ ERROR ──"));
});
}

@Test
public void showsLabelForNewModelEvents() {
IntegUtils.withTempDir("diff", dir -> {
Path a = dir.resolve("a.smithy");
writeFile(a, "$version: \"2.0\"\nnamespace example\nstring A\n");

Path b = dir.resolve("b.smithy");
writeFile(b, "$version: \"2.0\"\nnamespace example\n@aaaaaa\nstring A\n");

RunResult result = IntegUtils.run(dir, ListUtils.of("diff", "--old", a.toString(), "--new", b.toString()));
assertThat(result.getExitCode(), equalTo(1));
assertThat(result.getOutput(), containsString("── NEW ─ ERROR ──"));
});
}

@Test
public void showsLabelForDiffEvents() {
IntegUtils.withTempDir("diff", dir -> {
Path a = dir.resolve("a.smithy");
writeFile(a, "$version: \"2.0\"\nnamespace example\nstring A\n");

Path b = dir.resolve("b.smithy");
writeFile(b, "$version: \"2.0\"\nnamespace example\nstring A\nstring B\n"); // Added B.

RunResult result = IntegUtils.run(dir, ListUtils.of(
"diff",
"--old", a.toString(),
"--new", b.toString(),
"--severity", "NOTE")); // Note that this is required since the default severity is WARNING.
assertThat(result.getExitCode(), equalTo(0));
assertThat(result.getOutput(), containsString("── DIFF ─ NOTE ──"));
});
}

private void writeFile(Path path, String contents) {
try {
FileWriter fileWriter = new FileWriter(path.toString());
PrintWriter printWriter = new PrintWriter(fileWriter);
printWriter.print(contents);
printWriter.close();
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ private static List<String> createSmithyCommand(List<String> args) {
throw new RuntimeException("No SMITHY_BINARY location was set. Did you build the Smithy jlink CLI?");
}

private static void withTempDir(String name, Consumer<Path> consumer) {
static void withTempDir(String name, Consumer<Path> consumer) {
try {
Path path = Files.createTempDirectory(name.replace("/", "_"));
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,16 @@
import java.util.List;
import java.util.function.Consumer;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import software.amazon.smithy.build.model.SmithyBuildConfig;
import software.amazon.smithy.cli.ArgumentReceiver;
import software.amazon.smithy.cli.Arguments;
import software.amazon.smithy.cli.CliError;
import software.amazon.smithy.cli.ColorBuffer;
import software.amazon.smithy.cli.Command;
import software.amazon.smithy.cli.HelpPrinter;
import software.amazon.smithy.cli.StandardOptions;
import software.amazon.smithy.cli.Style;
import software.amazon.smithy.cli.dependencies.DependencyResolver;
import software.amazon.smithy.diff.ModelDiff;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.loader.ModelAssembler;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidatedResult;
import software.amazon.smithy.model.validation.ValidationEvent;
Expand Down Expand Up @@ -111,58 +107,37 @@ int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env) {
throw new CliError("Unexpected arguments: " + arguments.getPositional());
}

StandardOptions standardOptions = arguments.getReceiver(StandardOptions.class);
Options options = arguments.getReceiver(Options.class);
ClassLoader classLoader = env.classLoader();

List<String> oldModels = options.oldModels;
List<String> newModels = options.newModels;
LOGGER.fine(() -> String.format("Setting old models to: %s; new models to: %s", oldModels, newModels));

ModelAssembler assembler = ModelBuilder.createModelAssembler(classLoader);
Model oldModel = loadModel("old", assembler, oldModels);
assembler.reset();
Model newModel = loadModel("new", assembler, newModels);

ModelBuilder modelBuilder = new ModelBuilder()
.config(config)
.arguments(arguments)
.env(env)
.validationPrinter(env.stderr())
.validationMode(Validator.Mode.DISABLE)
.severity(Severity.DANGER);
Model oldModel = modelBuilder
.models(oldModels)
.titleLabel("OLD", Style.BG_BRIGHT_BLUE, Style.BLACK)
.build();
Model newModel = modelBuilder
.models(newModels)
.titleLabel("NEW", Style.BG_BRIGHT_BLUE, Style.BLACK)
.build();

// Diff the models and report on the events, failing if necessary.
List<ValidationEvent> events = ModelDiff.compare(classLoader, oldModel, newModel);
boolean hasError = events.stream().anyMatch(event -> event.getSeverity() == Severity.ERROR);
boolean hasDanger = events.stream().anyMatch(event -> event.getSeverity() == Severity.DANGER);
boolean hasWarning = events.stream().anyMatch(event -> event.getSeverity() == Severity.DANGER);
String result = events.stream().map(ValidationEvent::toString).collect(Collectors.joining("\n"));

if (hasError) {
throw new CliError(String.format("Model diff detected errors: %n%s", result));
}

if (!result.isEmpty()) {
env.stdout().println(result);
}

// Print the "framing" style output to stderr only if !quiet.
if (!standardOptions.quiet()) {
try (ColorBuffer buffer = ColorBuffer.of(env.colors(), env.stderr())) {
if (hasDanger) {
buffer.println("Smithy diff detected danger", Style.BRIGHT_RED, Style.BOLD);
} else if (hasWarning) {
buffer.println("Smithy diff detected warnings", Style.BRIGHT_YELLOW, Style.BOLD);
} else {
buffer.println("Smithy diff complete", Style.BRIGHT_GREEN, Style.BOLD);
}
}
}
modelBuilder
.titleLabel("DIFF", Style.BG_BRIGHT_BLACK, Style.WHITE)
.validatedResult(new ValidatedResult<>(newModel, events))
.severity(null) // reset so it takes on standard option settings.
.build();

return 0;
}

private Model loadModel(String descriptor, ModelAssembler assembler, List<String> models) {
models.forEach(assembler::addImport);
ValidatedResult<Model> result = assembler.assemble();
if (result.isBroken()) {
throw new CliError("Error loading " + descriptor + " models: \n" + result.getValidationEvents().stream()
.map(ValidationEvent::toString)
.collect(Collectors.joining("\n")));
}

return result.unwrap();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import software.amazon.smithy.cli.Command;
import software.amazon.smithy.cli.EnvironmentVariable;
import software.amazon.smithy.cli.StandardOptions;
import software.amazon.smithy.cli.Style;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.loader.ModelAssembler;
import software.amazon.smithy.model.loader.sourcecontext.SourceContextLoader;
Expand All @@ -57,13 +58,17 @@ final class ModelBuilder {
private Command.Env env;
private SmithyBuildConfig config;
private Severity severity;
private ValidatedResult<Model> validatedResult;
private String titleLabel;
private Style[] titleLabelStyles;

public ModelBuilder arguments(Arguments arguments) {
this.arguments = arguments;
return this;
}

public ModelBuilder models(List<String> models) {
validatedResult = null;
this.models = models;
return this;
}
Expand Down Expand Up @@ -93,6 +98,17 @@ public ModelBuilder severity(Severity severity) {
return this;
}

public ModelBuilder validatedResult(ValidatedResult<Model> validatedResult) {
this.validatedResult = validatedResult;
return this;
}

public ModelBuilder titleLabel(String titleLabel, Style... styles) {
this.titleLabel = titleLabel;
this.titleLabelStyles = styles;
return this;
}

public Model build() {
SmithyBuilder.requiredState("arguments", arguments);
SmithyBuilder.requiredState("models", models);
Expand All @@ -104,7 +120,6 @@ public Model build() {
DiscoveryOptions discoveryOptions = arguments.getReceiver(DiscoveryOptions.class);
Severity minSeverity = resolveMinSeverity(standardOptions);
ClassLoader classLoader = env.classLoader();
ModelAssembler assembler = createModelAssembler(classLoader);
ColorFormatter colors = env.colors();
CliPrinter stderr = env.stderr();

Expand All @@ -116,31 +131,38 @@ public Model build() {
validationMode = Validator.Mode.from(standardOptions);
}

if (validationMode == Validator.Mode.DISABLE) {
assembler.disableValidation();
}

// Emit status updates.
AtomicInteger issueCount = new AtomicInteger();
assembler.validationEventListener(createStatusUpdater(standardOptions, colors, stderr, issueCount));
if (validatedResult == null) {
ModelAssembler assembler = createModelAssembler(classLoader);

handleModelDiscovery(discoveryOptions, assembler, classLoader, config);
handleUnknownTraitsOption(buildOptions, assembler);
config.getSources().forEach(assembler::addImport);
models.forEach(assembler::addImport);
config.getImports().forEach(assembler::addImport);
ValidatedResult<Model> result = assembler.assemble();
if (validationMode == Validator.Mode.DISABLE) {
assembler.disableValidation();
}

clearStatusUpdateIfPresent(issueCount, stderr);
// Emit status updates.
AtomicInteger issueCount = new AtomicInteger();
assembler.validationEventListener(createStatusUpdater(standardOptions, colors, stderr, issueCount));

handleModelDiscovery(discoveryOptions, assembler, classLoader, config);
handleUnknownTraitsOption(buildOptions, assembler);
config.getSources().forEach(assembler::addImport);
models.forEach(assembler::addImport);
config.getImports().forEach(assembler::addImport);
validatedResult = assembler.assemble();
clearStatusUpdateIfPresent(issueCount, stderr);
}

// Sort events by file so that we can efficiently read files for context sequentially.
List<ValidationEvent> sortedEvents = new ArrayList<>(result.getValidationEvents());
List<ValidationEvent> sortedEvents = new ArrayList<>(validatedResult.getValidationEvents());
sortedEvents.sort(Comparator.comparing(ValidationEvent::getSourceLocation));

SourceContextLoader sourceContextLoader = result.getResult()
SourceContextLoader sourceContextLoader = validatedResult.getResult()
.map(model -> SourceContextLoader.createModelAwareLoader(model, DEFAULT_CODE_LINES))
.orElseGet(() -> SourceContextLoader.createLineBasedLoader(DEFAULT_CODE_LINES));
PrettyAnsiValidationFormatter formatter = new PrettyAnsiValidationFormatter(sourceContextLoader, colors);
PrettyAnsiValidationFormatter formatter = PrettyAnsiValidationFormatter.builder()
.sourceContextLoader(sourceContextLoader)
.colors(colors)
.titleLabel(titleLabel, titleLabelStyles)
.build();

for (ValidationEvent event : sortedEvents) {
// Only log events that are >= --severity. Note that setting --quiet inherently
Expand All @@ -151,13 +173,13 @@ public Model build() {
}

// Note: disabling validation will still show a summary of failures if the model can't be loaded.
Validator.validate(validationMode != Validator.Mode.ENABLE, colors, stderr, result);
Validator.validate(validationMode != Validator.Mode.ENABLE, colors, stderr, validatedResult);

// Flush outputs to ensure there is no interleaving with subsequent command output.
env.stderr().flush();
env.stdout().flush();

return result.getResult().orElseThrow(() -> new RuntimeException("Expected Validator to throw"));
return validatedResult.getResult().orElseThrow(() -> new RuntimeException("Expected Validator to throw"));
}

static Consumer<ValidationEvent> createStatusUpdater(
Expand Down
Loading

0 comments on commit dcd7a75

Please sign in to comment.