Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add git diff mode to CLI #1724

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Add git diff mode to CLI #1724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions smithy-cli/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,11 @@ task integ(type: Test) {
testClassesDirs = sourceSets["it"].output.classesDirs
classpath = sourceSets["it"].runtimeClasspath
maxParallelForks = Runtime.getRuntime().availableProcessors() / 2

testLogging {
events = ["passed", "skipped", "failed"]
exceptionFormat = "full"
}
}

// Runtime images need to be created before integration tests can run.
Expand Down
169 changes: 158 additions & 11 deletions smithy-cli/src/it/java/software/amazon/smithy/cli/DiffCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,19 @@

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

import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.UncheckedIOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import org.junit.jupiter.api.Test;
import software.amazon.smithy.utils.IoUtils;
import software.amazon.smithy.utils.ListUtils;

public class DiffCommandTest {
Expand All @@ -38,7 +40,7 @@ public void passingDiffEventsExitZero() {
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));
assertThat("Not 0: output [" + result.getOutput() + ']', result.getExitCode(), is(0));
});
}

Expand All @@ -49,7 +51,7 @@ public void showsLabelForOldModelEvents() {
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("Not 1: output [" + result.getOutput() + ']', result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("── OLD ERROR ──"));
});
}
Expand All @@ -64,7 +66,7 @@ public void showsLabelForNewModelEvents() {
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("Not 1: output [" + result.getOutput() + ']', result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("── NEW ERROR ──"));
});
}
Expand All @@ -83,7 +85,7 @@ public void showsLabelForDiffEvents() {
"--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("Not 0: output [" + result.getOutput() + ']', result.getExitCode(), is(0));
assertThat(result.getOutput(), containsString("── DIFF NOTE ──"));
});
}
Expand All @@ -110,15 +112,15 @@ public void doesNotUseImportsOrSourcesWithArbitraryMode() {
"-c", dir.resolve("smithy-build.json").toString(),
"--old", file.toString(),
"--new", file.toString()));
assertThat(result.getExitCode(), equalTo(0));
assertThat("Not 0: output [" + result.getOutput() + ']', result.getExitCode(), is(0));
});
}

@Test
public void requiresOldAndNewForArbitraryMode() {
RunResult result = IntegUtils.run(Paths.get("."), ListUtils.of("diff"));

assertThat(result.getExitCode(), is(1));
assertThat("Not 1: output [" + result.getOutput() + ']', result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("Missing required --old argument"));
}

Expand All @@ -129,7 +131,7 @@ public void doesNotAllowNewWithProjectMode() {
"--new", "x",
"--old", "y"));

assertThat(result.getExitCode(), is(1));
assertThat("Not 1: output [" + result.getOutput() + ']', result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("--new cannot be used with this diff mode"));
}

Expand All @@ -143,8 +145,9 @@ public void projectModeUsesConfigOfOldModel() {
"project",
"--old",
outer.toString()));

assertThat("Not 1: output [" + result.getOutput() + ']', result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("ChangedShapeType"));
assertThat(result.getExitCode(), equalTo(1));
});
});
}
Expand All @@ -159,7 +162,151 @@ public void projectModeCanDiffAgainstSingleFile() {
"project",
"--old",
dir.resolve("model").resolve("main.smithy").toString()));
assertThat(result.getExitCode(), equalTo(0));

assertThat("Not 0: output [" + result.getOutput() + ']', result.getExitCode(), is(0));
});
}

@Test
public void gitDiffAgainstLastCommit() {
// Diff against itself (the only model file of the project), so there should be no differences.
IntegUtils.withProject("simple-config-sources", dir -> {
initRepo(dir);
commitChanges(dir);

RunResult result = runDiff(dir);

assertThat("Not 0: output [" + result.getOutput() + ']', result.getExitCode(), is(0));
});
}

private RunResult runDiff(Path dir) {
return runDiff(dir, null);
}

private RunResult runDiff(Path dir, String oldCommit) {
List<String> args = new ArrayList<>();
args.add("diff");
args.add("--mode");
args.add("git");
if (oldCommit != null) {
args.add("--old");
args.add(oldCommit);
}

return IntegUtils.run(dir, args);
}

@Test
public void canDiffAgainstLastCommitWithFailure() {
IntegUtils.withProject("simple-config-sources", dir -> {
initRepo(dir);
commitChanges(dir);

Path file = dir.resolve("model").resolve("main.smithy");
writeFile(file, "$version: \"2.0\"\nnamespace smithy.example\ninteger MyString\n");
RunResult result = IntegUtils.run(dir, ListUtils.of("diff", "--mode", "git"));

assertThat("Not 1: output [" + result.getOutput() + ']', result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("ChangedShapeType"));
});
}

private void initRepo(Path dir) {
run(ListUtils.of("git", "init"), dir);
run(ListUtils.of("git", "config", "user.email", "[email protected]"), dir);
run(ListUtils.of("git", "config", "user.name", "Your Name"), dir);
}

private void commitChanges(Path dir) {
run(ListUtils.of("git", "add", "-A"), dir);
run(ListUtils.of("git", "commit", "-m", "Foo"), dir);
}

private void run(List<String> args, Path root) {
StringBuilder output = new StringBuilder();
int result = IoUtils.runCommand(args, root, output, Collections.emptyMap());
if (result != 0) {
throw new RuntimeException("Error running command: " + args + ": " + output);
}
}

@Test
public void gitDiffAgainstLastCommitAfterClean() {
IntegUtils.withProject("simple-config-sources", dir -> {
initRepo(dir);
commitChanges(dir);
RunResult result = runDiff(dir);
assertThat("Not zero: output [" + result.getOutput() + ']', result.getExitCode(), is(0));

// This will remove the previously created worktree.
IntegUtils.run(dir, ListUtils.of("clean"));

// Running diff again ensures that we handle the case where there's a prunable worktree.
result = runDiff(dir);

assertThat("Not zero: output [" + result.getOutput() + ']', result.getExitCode(), is(0));
});
}

@Test
public void gitDiffEnsuresHeadUpdatesAsCommitsAreMade() {
IntegUtils.withProject("simple-config-sources", dir -> {
initRepo(dir);
commitChanges(dir);

// Run with HEAD (the default)
RunResult result = runDiff(dir);
assertThat("Not zero: output [" + result.getOutput() + ']', result.getExitCode(), is(0));

// Now make another commit, which updates HEAD and ensures the worktree updates too.
Path file = dir.resolve("model").resolve("main.smithy");
writeFile(file, "$version: \"2.0\"\nnamespace smithy.example\ninteger MyString\n");
commitChanges(dir);

// Run diff again, which won't fail because the last commit is the change that broke the model, meaning
// the current state of the model is valid.
result = runDiff(dir);

assertThat("Not zero: output [" + result.getOutput() + ']', result.getExitCode(), is(0));
});
}

@Test
public void gitDiffAgainstSpecificCommit() {
IntegUtils.withProject("simple-config-sources", dir -> {
initRepo(dir);
commitChanges(dir);

// Run with explicit HEAD
RunResult result = runDiff(dir, "HEAD");
assertThat("Not zero: output [" + result.getOutput() + ']', result.getExitCode(), is(0));

// Now make another commit, which updates HEAD and ensures the worktree updates too.
Path file = dir.resolve("model").resolve("main.smithy");
writeFile(file, "$version: \"2.0\"\nnamespace smithy.example\ninteger MyString\n");
commitChanges(dir);

// Run diff again, which won't fail because the last commit is the change that broke the model, meaning
// the current state of the model is valid.
result = runDiff(dir, "HEAD~1");

assertThat("Not 1: output [" + result.getOutput() + ']', result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("ChangedShapeType"));
});
}

@Test
public void gitModeDoesNotAllowNewArgument() {
IntegUtils.withProject("simple-config-sources", dir -> {
initRepo(dir);
commitChanges(dir);

// Now that it's running in a git repo, it won't fail due to that and will validate that --new is invalid.
RunResult result = IntegUtils.run(dir, ListUtils.of("diff", "--mode", "git", "--new", "some-file.smithy"));

assertThat("Not 1: output [" + result.getOutput() + ']', result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("--new cannot be used with this diff mode"));
});
}
}
10 changes: 9 additions & 1 deletion smithy-cli/src/it/java/software/amazon/smithy/cli/IntegUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,15 @@
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;
import java.util.logging.Level;
import java.util.logging.Logger;
import software.amazon.smithy.utils.IoUtils;
import software.amazon.smithy.utils.MapUtils;

public final class IntegUtils {

private static final Logger LOGGER = Logger.getLogger(IntegUtils.class.getName());

private IntegUtils() {}

public static void withProject(String projectName, Consumer<Path> consumer) {
Expand Down Expand Up @@ -97,7 +101,11 @@ static void withTempDir(String name, Consumer<Path> consumer) {
try {
consumer.accept(path);
} finally {
IoUtils.rmdir(path);
try {
IoUtils.rmdir(path);
} catch (Exception e) {
LOGGER.log(Level.INFO, "Unable to delete temp directory", e);
}
}
} catch (IOException e) {
throw new UncheckedIOException(e);
Expand Down
6 changes: 6 additions & 0 deletions smithy-cli/src/main/java/software/amazon/smithy/cli/HelpPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,12 @@ LineWrapper space() {
LineWrapper appendWithinLine(String text) {
if (column + text.length() > maxLength) {
newLine();
// If the text starts with a space, then eat the space since it isn't needed to separate words now.
if (text.startsWith(" ")) {
builder.append(text, 1, text.length());
column += text.length() - 1;
return this;
}
}

builder.append(text);
Expand Down
21 changes: 21 additions & 0 deletions smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@

package software.amazon.smithy.cli.commands;

import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.function.Consumer;
import software.amazon.smithy.build.SmithyBuild;
import software.amazon.smithy.build.model.SmithyBuildConfig;
import software.amazon.smithy.cli.ArgumentReceiver;
import software.amazon.smithy.cli.HelpPrinter;

Expand Down Expand Up @@ -70,4 +74,21 @@ String output() {
void noPositionalArguments(boolean noPositionalArguments) {
this.noPositionalArguments = noPositionalArguments;
}

/**
* Resolves the correct build directory by looking at the --output argument, outputDirectory config setting,
* and finally the default build directory.
*
* @param config Config to check.
* @return Returns the resolved build directory.
*/
Path resolveOutput(SmithyBuildConfig config) {
if (output != null) {
return Paths.get(output);
} else {
return config.getOutputDirectory()
.map(Paths::get)
.orElseGet(SmithyBuild::getDefaultOutputDirectory);
}
}
}
12 changes: 5 additions & 7 deletions smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ClasspathAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@

import java.io.File;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;
import java.util.logging.Logger;
import software.amazon.smithy.build.SmithyBuild;
import software.amazon.smithy.build.model.MavenConfig;
import software.amazon.smithy.build.model.MavenRepository;
import software.amazon.smithy.build.model.SmithyBuildConfig;
Expand Down Expand Up @@ -134,7 +132,9 @@ private List<Path> resolveDependencies(
DependencyResolver baseResolver = dependencyResolverFactory.create(smithyBuildConfig, env);
long lastModified = smithyBuildConfig.getLastModifiedInMillis();
DependencyResolver delegate = new FilterCliVersionResolver(SmithyCli.getVersion(), baseResolver);
DependencyResolver resolver = new FileCacheResolver(getCacheFile(buildOptions), lastModified, delegate);
DependencyResolver resolver = new FileCacheResolver(getCacheFile(buildOptions, smithyBuildConfig),
lastModified,
delegate);
addConfiguredMavenRepos(smithyBuildConfig, resolver);
maven.getDependencies().forEach(resolver::addDependency);
List<ResolvedArtifact> artifacts = resolver.resolve();
Expand Down Expand Up @@ -171,9 +171,7 @@ private static void addConfiguredMavenRepos(SmithyBuildConfig config, Dependency
}
}

private File getCacheFile(BuildOptions buildOptions) {
String output = buildOptions.output();
Path buildPath = output == null ? SmithyBuild.getDefaultOutputDirectory() : Paths.get(output);
return buildPath.resolve("classpath.json").toFile();
private File getCacheFile(BuildOptions buildOptions, SmithyBuildConfig config) {
return buildOptions.resolveOutput(config).resolve("classpath.json").toFile();
}
}
Loading