Skip to content

Commit

Permalink
New config format (#523)
Browse files Browse the repository at this point in the history
* remove tests cmd & linter parameters since we no longer have dynamic analysis

* new config format

* validate excluded classes names
  • Loading branch information
Feuermagier committed May 11, 2024
1 parent 04ce087 commit 1b1f3cd
Show file tree
Hide file tree
Showing 10 changed files with 282 additions and 253 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
import de.firemage.autograder.cmd.output.Annotation;
import de.firemage.autograder.core.ArtemisUtil;
import de.firemage.autograder.core.CheckConfiguration;
import de.firemage.autograder.core.CodePosition;
import de.firemage.autograder.core.Linter;
import de.firemage.autograder.core.LinterConfigurationException;
import de.firemage.autograder.core.LinterException;
import de.firemage.autograder.core.LinterStatus;
import de.firemage.autograder.core.Problem;
Expand Down Expand Up @@ -63,9 +66,6 @@ public class Application implements Callable<Integer> {
@Parameters(index = "1", description = "The root folder which contains the files to check.")
private Path file;

@Parameters(index = "2", defaultValue = "", description = "The root folder which contains the tests to run. If not provided or empty, no tests will be run.")
private Path tests;

@Option(names = {"-j", "--java", "--java-version"}, defaultValue = "17", description = "Set the Java version.")
private String javaVersion;

Expand All @@ -91,11 +91,9 @@ public class Application implements Callable<Integer> {
private CommandSpec spec;

private final TempLocation tempLocation;
private final Set<String> exludedClasses;

public Application(TempLocation tempLocation) {
this.tempLocation = tempLocation;
this.exludedClasses = new HashSet<>();
}

private static Charset getConsoleCharset() {
Expand Down Expand Up @@ -131,12 +129,12 @@ private static Highlight highlightFromCodePosition(CodePosition codePosition, St

private void execute(
Linter linter,
List<ProblemType> checks,
CheckConfiguration checkConfiguration,
UploadedFile uploadedFile,
Consumer<LinterStatus> statusConsumer
) throws LinterException, IOException {
if (outputJson) {
List<Problem> problems = linter.checkFile(uploadedFile, tests, checks, statusConsumer);
List<Problem> problems = linter.checkFile(uploadedFile, checkConfiguration, statusConsumer);
System.out.println(">> Problems <<");
printProblemsAsJson(problems, linter);
return;
Expand All @@ -146,7 +144,7 @@ private void execute(
CmdUtil.beginSection("Checks");
ProgressAnimation progress = new ProgressAnimation("Checking...");
progress.start();
List<Problem> problems = linter.checkFile(uploadedFile, tests, checks, statusConsumer);
List<Problem> problems = linter.checkFile(uploadedFile, checkConfiguration, statusConsumer);
progress.finish("Completed checks");

if (problems.isEmpty()) {
Expand Down Expand Up @@ -181,55 +179,23 @@ private void execute(
CmdUtil.beginSection("Checks");
ProgressAnimation progress = new ProgressAnimation("Checking...");
progress.start();
List<Problem> problems = linter.checkFile(uploadedFile, tests, checks, statusConsumer);
List<Problem> problems = linter.checkFile(uploadedFile, checkConfiguration, statusConsumer);
progress.finish("Completed checks");

printProblems(problems, linter);

CmdUtil.endSection();
}

private List<ProblemType> getChecks() throws IOException {
List<String> checks;
if (passConfig) {
checks = List.of(new ObjectMapper(new YAMLFactory()).readValue(checkConfig, String[].class));
} else {
checks = List.of(new ObjectMapper(new YAMLFactory()).readValue(new File(checkConfig), String[].class));
}

List<ProblemType> result = new ArrayList<>();

// HACK: EXCLUDE is used to ignore some classes, blame the config format for the hacky solution
for (String check : checks) {
if (check.startsWith("EXCLUDE")) {
this.exludedClasses.addAll(List.of(check.substring("EXCLUDE".length() + 1).split(" ")));
continue;
}

try {
result.add(ProblemType.valueOf(check));
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Unknown check '%s'".formatted(check), e);
}
}

return result;
}

@Override
public Integer call() {
if (!JavaVersion.isValidJavaVersion(javaVersion)) {
throw new ParameterException(this.spec.commandLine(), "Unknown java version '" + javaVersion + "'");
}

if (this.artemisFolders) {
try (Stream<Path> files = Files.list(this.file)) {
this.file = files
.filter(child -> !child.endsWith(".metadata"))
.findAny()
.orElseThrow(() -> new IllegalStateException("No student code found"))
.resolve("assignment")
.resolve("src");
try {
this.file = ArtemisUtil.resolveCodePathEclipseGradingTool(this.file);
} catch (IOException e) {
e.printStackTrace();
return IO_EXIT_CODE;
Expand All @@ -240,10 +206,15 @@ public Integer call() {
System.out.println("Student source code directory is " + file);
}

List<ProblemType> checks;
// Create the check configuration
CheckConfiguration checkConfiguration;
try {
checks = this.getChecks();
} catch (IOException e) {
if (passConfig) {
checkConfiguration = CheckConfiguration.fromConfigString(checkConfig);
} else {
checkConfiguration = CheckConfiguration.fromConfigFile(Path.of(checkConfig));
}
} catch (IOException | LinterConfigurationException e) {
e.printStackTrace();
return IO_EXIT_CODE;
}
Expand All @@ -252,7 +223,6 @@ public Integer call() {
.threads(0)
.tempLocation(this.tempLocation)
.maxProblemsPerCheck(this.maxProblemsPerCheck)
.exclude(problem -> this.exludedClasses.contains(problem.getPosition().file().getName().replace(".java", "")))
.build();

Consumer<LinterStatus> statusConsumer = status ->
Expand All @@ -269,7 +239,7 @@ public Integer call() {
this.tempLocation,
statusConsumer,
null)) {
this.execute(linter, checks, uploadedFile, statusConsumer);
this.execute(linter, checkConfiguration, uploadedFile, statusConsumer);
} catch (CompilationFailureException e) {
CmdUtil.printlnErr("Compilation failed: " + e.getMessage());
return COMPILATION_EXIT_CODE;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package de.firemage.autograder.core;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.stream.Stream;

public class ArtemisUtil {
private ArtemisUtil() {

}

public static Path resolveCodePathEclipseGradingTool(Path submissionRoot) throws IOException {
try (Stream<Path> files = Files.list(submissionRoot)) {
return files
.filter(child -> !child.endsWith(".metadata"))
.findAny()
.orElseThrow(() -> new IllegalStateException("No student code found"))
.resolve("assignment")
.resolve("src");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package de.firemage.autograder.core;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;

public record CheckConfiguration(List<ProblemType> problemsToReport, List<String> excludedClasses) {
public static CheckConfiguration empty() {
return new CheckConfiguration(List.of(), List.of());
}

public static CheckConfiguration fromConfigFile(Path configFile) throws IOException, LinterConfigurationException {
return CheckConfiguration.fromConfigString(Files.readString(configFile));
}

public static CheckConfiguration fromConfigString(String configString) throws IOException, LinterConfigurationException {
var config = new ObjectMapper(new YAMLFactory()).readValue(configString, CheckConfiguration.class);
config.validate();
return config;
}

public static CheckConfiguration fromProblemTypes(List<ProblemType> problemsToReport) {
return new CheckConfiguration(problemsToReport, List.of());
}

private void validate() throws LinterConfigurationException {
if (this.excludedClasses != null) {
for (String excludedClass : this.excludedClasses) {
if (excludedClass.contains("/") || excludedClass.contains(".")) {
throw new LinterConfigurationException("Invalid excluded class name '%s'. Please check your configuration.".formatted(excludedClass));
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,13 @@ public final class Linter {
private final FluentBundle fluentBundle;
private final ClassLoader classLoader;
private final int maxProblemsPerCheck;
private final Predicate<Problem> isExcluded;

private Linter(
Locale locale,
TempLocation tempLocation,
int threads,
ClassLoader classLoader,
int maxProblemsPerCheck,
Predicate<Problem> isExcluded
int maxProblemsPerCheck
) {
String filename = switch (locale.getLanguage()) {
case "de" -> "/strings.de.ftl";
Expand All @@ -73,7 +71,6 @@ private Linter(
this.threads = threads;
this.classLoader = classLoader;
this.maxProblemsPerCheck = maxProblemsPerCheck;
this.isExcluded = isExcluded;
}

public static class Builder {
Expand All @@ -82,11 +79,9 @@ public static class Builder {
private int threads;
private ClassLoader classLoader;
private int maxProblemsPerCheck = -1;
private Predicate<Problem> isExcluded;

private Builder(Locale locale) {
this.locale = locale;
this.isExcluded = problem -> false;
}

public Builder tempLocation(TempLocation tempLocation) {
Expand All @@ -109,11 +104,6 @@ public Builder classLoader(ClassLoader classLoader) {
return this;
}

public Builder exclude(Predicate<Problem> isExcluded) {
this.isExcluded = isExcluded;
return this;
}

public Linter build() {
TempLocation tempLocation = this.tempLocation;

Expand All @@ -126,8 +116,7 @@ public Linter build() {
tempLocation,
this.threads,
this.classLoader,
this.maxProblemsPerCheck,
this.isExcluded
this.maxProblemsPerCheck
);
}
}
Expand All @@ -145,24 +134,18 @@ public FluentBundle getFluentBundle() {
}

public List<Problem> checkFile(
UploadedFile file, Path tests,
List<ProblemType> problemsToReport,
UploadedFile file,
CheckConfiguration checkConfiguration,
Consumer<LinterStatus> statusConsumer
) throws LinterException, IOException {
return this.checkFile(
file,
tests,
problemsToReport,
findChecksForProblemTypes(problemsToReport),
statusConsumer
);
var checks = this.findChecksForProblemTypes(checkConfiguration.problemsToReport());
return this.checkFile(file, checkConfiguration, checks, statusConsumer);
}

public List<Problem> checkFile(
UploadedFile file,
Path tests,
Collection<ProblemType> problemsToReport,
Iterable<? extends Check> checks,
CheckConfiguration checkConfiguration,
List<? extends Check> checks,
Consumer<LinterStatus> statusConsumer
) throws LinterException, IOException {
// the file is null if the student did not upload source code
Expand Down Expand Up @@ -241,18 +224,22 @@ public List<Problem> checkFile(
}

List<Problem> unreducedProblems = result.problems();
if (!problemsToReport.isEmpty()) {
if (!checkConfiguration.problemsToReport().isEmpty()) {
unreducedProblems = result
.problems()
.stream()
.filter(problem -> problemsToReport.contains(problem.getProblemType()))
.filter(problem -> checkConfiguration.problemsToReport().contains(problem.getProblemType()))
.toList();
}

// filter out excluded problems:
unreducedProblems = unreducedProblems.stream()
.filter(this.isExcluded.negate())
.toList();
// filter out problems in excluded classes
var excludedClasses = checkConfiguration.excludedClasses();
if (excludedClasses != null && !excludedClasses.isEmpty()) {
unreducedProblems = unreducedProblems.stream()
.filter(problem -> !checkConfiguration.excludedClasses()
.contains(problem.getPosition().file().getName().replace(".java", "")))
.toList();
}

return this.mergeProblems(unreducedProblems);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package de.firemage.autograder.core;

public class LinterConfigurationException extends LinterException {
public LinterConfigurationException(String message) {
super(message);
}

public LinterConfigurationException(String message, Throwable cause) {
super(message, cause);
}

public LinterConfigurationException(Throwable cause) {
super(cause);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,7 @@ Stream<DynamicTest> createCheckTest() throws URISyntaxException, IOException {

var problems = linter.checkFile(
file,
testInput.path().resolve("tests"),
List.of(),
CheckConfiguration.empty(),
List.of(check),
status -> {
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
package de.firemage.autograder.core;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
import org.junit.jupiter.api.Test;

import java.io.File;
import java.io.IOException;
import java.nio.file.Path;
import java.util.ArrayList;
Expand Down Expand Up @@ -36,11 +33,10 @@ private static <T extends Comparable<? super T>> void assertProblemTypes(List<T>
}

@Test
void hasAllProblemTypes() throws IOException {
void hasAllProblemTypes() throws IOException, LinterConfigurationException {
// the `System.getProperty("user.dir")` is the path to the autograder-core directory
Path path = Path.of(System.getProperty("user.dir"), "..", "sample_config.yaml");
List<ProblemType> present = List.of(
new ObjectMapper(new YAMLFactory()).readValue(new File(path.toUri()), ProblemType[].class));
List<ProblemType> present = CheckConfiguration.fromConfigFile(path).problemsToReport();

assertProblemTypes(Arrays.asList(ProblemType.values()), present);
}
Expand Down
Loading

0 comments on commit 1b1f3cd

Please sign in to comment.