From 29b9ab2cfa72112001ed177e018f59ef14fe47f8 Mon Sep 17 00:00:00 2001 From: kimmoal Date: Wed, 13 Dec 2017 11:34:38 +0200 Subject: [PATCH] Fix path issues and point parsing (#90) * Fix point parsing regex Points with a '.' were not parsed * Fix path issues, improve error handling/logging * Tests for point parsing * Fix checkstyle * Replace RuntimeExceptions with RunResults --- .../tmc/langs/qmake/QmakeBuildException.java | 11 --- .../cs/tmc/langs/qmake/QmakePlugin.java | 63 ++++++++++---- .../cs/tmc/langs/qmake/QmakePluginTest.java | 86 ++++++++++++++++++- .../passing_multiple_lib_same_point.pro | 8 +- .../{test_case_app => src}/main.cpp | 0 .../test_case_app.pro => src/src.pro} | 0 .../test_case_test_runner.cpp | 11 ++- 7 files changed, 141 insertions(+), 38 deletions(-) delete mode 100644 tmc-langs-qmake/src/main/java/fi/helsinki/cs/tmc/langs/qmake/QmakeBuildException.java rename tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/{test_case_app => src}/main.cpp (100%) rename tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/{test_case_app/test_case_app.pro => src/src.pro} (100%) diff --git a/tmc-langs-qmake/src/main/java/fi/helsinki/cs/tmc/langs/qmake/QmakeBuildException.java b/tmc-langs-qmake/src/main/java/fi/helsinki/cs/tmc/langs/qmake/QmakeBuildException.java deleted file mode 100644 index 2183f0bd7..000000000 --- a/tmc-langs-qmake/src/main/java/fi/helsinki/cs/tmc/langs/qmake/QmakeBuildException.java +++ /dev/null @@ -1,11 +0,0 @@ - -package fi.helsinki.cs.tmc.langs.qmake; - - -public class QmakeBuildException extends RuntimeException { - - QmakeBuildException(java.lang.Exception exception) { - System.err.println(exception); - } - -} diff --git a/tmc-langs-qmake/src/main/java/fi/helsinki/cs/tmc/langs/qmake/QmakePlugin.java b/tmc-langs-qmake/src/main/java/fi/helsinki/cs/tmc/langs/qmake/QmakePlugin.java index 37eb1bc0a..3ef5f55f4 100644 --- a/tmc-langs-qmake/src/main/java/fi/helsinki/cs/tmc/langs/qmake/QmakePlugin.java +++ b/tmc-langs-qmake/src/main/java/fi/helsinki/cs/tmc/langs/qmake/QmakePlugin.java @@ -31,6 +31,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.FileVisitResult; import java.nio.file.Files; +import java.nio.file.LinkOption; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; @@ -49,9 +50,9 @@ public final class QmakePlugin extends AbstractLanguagePlugin { private static final Path TMC_TEST_RESULTS = Paths.get("tmc_test_results.xml"); - // Finds pattern 'POINT(exercise_name, 1)' + // Finds pattern 'POINT(exercise_name, 1.1)' private static final Pattern POINT_PATTERN - = Pattern.compile("POINT\\(\\s*(\\w+),\\s*(\\w+)\\s*\\)\\s*;"); + = Pattern.compile("POINT\\(\\s*(\\w+),\\s*([^\\s|\\)]+)\\s*\\)\\s*;"); // Pattern to find comments private static final Pattern COMMENT_PATTERN = Pattern.compile("(^[^\"\\r\\n]*\\/\\*{1,2}.*?\\*\\/" @@ -79,8 +80,9 @@ public String getPluginName() { * Resolve the exercise .pro file from exercise directory. The file should * be named after the directory. */ - private Path getProFile(Path basePath) { - return Paths.get(basePath.toString() + "/" + basePath.getFileName() + ".pro"); + private Path getProFile(Path basePath) throws IOException { + Path fullPath = basePath.toRealPath(LinkOption.NOFOLLOW_LINKS); + return fullPath.resolve(fullPath.getFileName() + ".pro"); } @Override @@ -101,7 +103,11 @@ public Optional scanExercise(Path path, String exerciseName) { @Override public boolean isExerciseTypeCorrect(Path path) { - return Files.isRegularFile(getProFile(path)); + try { + return Files.isRegularFile(getProFile(path)); + } catch (IOException e) { + return false; + } } /** @@ -121,31 +127,45 @@ protected StudentFilePolicy getStudentFilePolicy(Path projectPath) { @Override public RunResult runTests(Path path) { - Path shadowDir = makeShadowBuildDir(path); + Path fullPath; + try { + fullPath = path.toRealPath(LinkOption.NOFOLLOW_LINKS); + } catch (IOException e) { + log.error("Exercise directory not found", e); + return filledFailure(Status.GENERIC_ERROR, "Exercise directory not found"); + } + + Path shadowDir; + try { + shadowDir = makeShadowBuildDir(fullPath); + } catch (IOException e) { + log.error("Preparing exercise failed", e); + return filledFailure(Status.GENERIC_ERROR, "Could not create build directory"); + } try { ProcessResult qmakeBuild = buildWithQmake(shadowDir); if (qmakeBuild.statusCode != 0) { log.error("Building project with qmake failed: {}", qmakeBuild.errorOutput); - return filledFailure(qmakeBuild.errorOutput); + return filledFailure(Status.COMPILE_FAILED, qmakeBuild.errorOutput); } } catch (IOException | InterruptedException e) { log.error("Building project with qmake failed", e); - throw new QmakeBuildException(e); + return filledFailure(Status.GENERIC_ERROR, "Building project with qmake failed"); } try { ProcessResult makeBuild = buildWithMake(shadowDir); if (makeBuild.statusCode != 0) { log.error("Building project with make failed: {}", makeBuild.errorOutput); - return filledFailure(makeBuild.errorOutput); + return filledFailure(Status.COMPILE_FAILED, makeBuild.errorOutput); } } catch (IOException | InterruptedException e) { log.error("Building project with make failed", e); - throw new QmakeBuildException(e); + return filledFailure(Status.GENERIC_ERROR, "Building project with make failed"); } - Path testResults = path.resolve(TMC_TEST_RESULTS); + Path testResults = shadowDir.resolve(TMC_TEST_RESULTS); String target = "check"; String config = "TESTARGS=-o " + testResults.toString() + ",xml"; @@ -158,11 +178,11 @@ public RunResult runTests(Path path) { if (!Files.exists(testResults)) { log.error("Failed to get test output at {}", testResults); - return filledFailure(testRun.output); + return filledFailure(Status.GENERIC_ERROR, testRun.output); } } catch (IOException | InterruptedException e) { log.error("Testing with make check failed", e); - throw new QmakeBuildException(e); + return filledFailure(Status.GENERIC_ERROR, "Testing with make check failed"); } QTestResultParser parser = new QTestResultParser(); @@ -185,11 +205,18 @@ public Map> getValidationErrors() { }; } - private Path makeShadowBuildDir(Path dir) { - File buildDir = dir.resolve("build").toFile(); + private Path makeShadowBuildDir(Path dir) throws IOException { + Path shadowPath = dir.resolve("build"); + if (Files.exists(shadowPath)) { + log.info("Shadow dir already exists at {}", shadowPath); + return shadowPath; + } + + File buildDir = shadowPath.toFile(); + log.info("Making shadow build dir to {}", buildDir.toPath()); if (!buildDir.mkdirs()) { - throw new RuntimeException( + throw new IOException( "Unable to create shadow build directory: " + buildDir.toPath()); } @@ -231,13 +258,13 @@ private ProcessResult run(String[] command, Path dir) throws IOException, Interr return runner.call(); } - private RunResult filledFailure(String output) { + private RunResult filledFailure(Status status, String output) { byte[] errorOutput = output.getBytes(StandardCharsets.UTF_8); ImmutableMap logs = new ImmutableMap.Builder() .put(SpecialLogs.COMPILER_OUTPUT, errorOutput) .build(); - return new RunResult(Status.COMPILE_FAILED, ImmutableList.of(), logs); + return new RunResult(status, ImmutableList.of(), logs); } /** diff --git a/tmc-langs-qmake/src/test/java/fi/helsinki/cs/tmc/langs/qmake/QmakePluginTest.java b/tmc-langs-qmake/src/test/java/fi/helsinki/cs/tmc/langs/qmake/QmakePluginTest.java index 8215440a0..9112c9127 100644 --- a/tmc-langs-qmake/src/test/java/fi/helsinki/cs/tmc/langs/qmake/QmakePluginTest.java +++ b/tmc-langs-qmake/src/test/java/fi/helsinki/cs/tmc/langs/qmake/QmakePluginTest.java @@ -7,6 +7,7 @@ import fi.helsinki.cs.tmc.langs.domain.ExerciseDesc; import fi.helsinki.cs.tmc.langs.domain.RunResult; import fi.helsinki.cs.tmc.langs.domain.TestDesc; +import fi.helsinki.cs.tmc.langs.domain.TestResult; import fi.helsinki.cs.tmc.langs.utils.TestUtils; import com.google.common.base.Optional; @@ -20,6 +21,7 @@ public class QmakePluginTest { private final QmakePlugin qmakePlugin; public QmakePluginTest() { + TestUtils.skipIfNotAvailable("qmake"); qmakePlugin = new QmakePlugin(); } @@ -49,6 +51,37 @@ public void testQtTestsPassingWithMultipleLib() { runTests("passing_multiple_lib")); } + @Test + public void testQtTestsPassingWithMultipleLibSamePoints() { + RunResult result = runTests(TestUtils.getPath(getClass(), + "passing_multiple_lib_same_point")); + assertEquals("Tests passing with multiple libraries and same points", + RunResult.Status.PASSED, result.status); + + assertEquals(3, result.testResults.size()); + + TestResult test1 = result.testResults.get(0); + assertEquals(test1.getName(), "test_function_one_here"); + assertTrue(test1.isSuccessful()); + assertEquals(2, test1.points.size()); + assertEquals("testPoint1", test1.points.get(0)); + assertEquals("testPoint1.1", test1.points.get(1)); + + TestResult test2 = result.testResults.get(1); + assertEquals(test2.getName(), "test_function_two_here"); + assertTrue(test2.isSuccessful()); + assertEquals(2, test2.points.size()); + assertEquals("testPoint1", test2.points.get(0)); + assertEquals("testPoint1.2", test2.points.get(1)); + + TestResult test3 = result.testResults.get(2); + assertEquals(test3.getName(), "test_function_two_here_2"); + assertTrue(test3.isSuccessful()); + assertEquals(2, test3.points.size()); + assertEquals("testPoint1", test3.points.get(0)); + assertEquals("testPoint1.3", test3.points.get(1)); + } + @Test public void testQtBuildFailingWithSingleLib() { assertEquals("Student source not compiling with compiling single lib", @@ -58,9 +91,24 @@ public void testQtBuildFailingWithSingleLib() { @Test public void testQtFailingSingleLib() { + RunResult result = runTests(TestUtils.getPath(getClass(), "failing_single_lib")); assertEquals("Tests failing with single library", RunResult.Status.TESTS_FAILED, - runTests("failing_single_lib")); + result.status); + + assertEquals(2, result.testResults.size()); + + TestResult test1 = result.testResults.get(0); + assertEquals(test1.getName(), "test_function_one_here"); + assertTrue(test1.isSuccessful()); + assertEquals(1, test1.points.size()); + assertEquals("1", test1.points.get(0)); + + TestResult test2 = result.testResults.get(1); + assertEquals(test2.getName(), "test_function_two_here"); + assertFalse(test2.isSuccessful()); + assertEquals(1, test2.points.size()); + assertEquals("2", test2.points.get(0)); } @Test @@ -127,6 +175,38 @@ public void testScanExerciseWithPassingSamePoints() { assertEquals("2", test2.points.get(0)); } + @Test + public void testScanExerciseWithPartialPoints() { + Optional optional = scanExercise("passing_multiple_lib_same_point"); + assertTrue(optional.isPresent()); + + ExerciseDesc desc = optional.get(); + + assertEquals("passing_multiple_lib_same_point", desc.name); + + assertEquals(3, desc.tests.size()); + TestDesc test1 = desc.tests.get(2); + TestDesc test2 = desc.tests.get(0); + TestDesc test3 = desc.tests.get(1); + + assertEquals("test_function_one_here", test1.name); + assertEquals("test_function_two_here", test2.name); + assertEquals("test_function_two_here_2", test3.name); + + assertEquals(2, test1.points.size()); + assertEquals("testPoint1", test1.points.get(0)); + assertEquals("testPoint1.1", test1.points.get(1)); + + assertEquals(2, test2.points.size()); + assertEquals("testPoint1", test2.points.get(0)); + assertEquals("testPoint1.2", test2.points.get(1)); + + assertEquals(2, test2.points.size()); + assertEquals("testPoint1", test3.points.get(0)); + assertEquals("testPoint1.3", test3.points.get(1)); + + } + @Test public void testScanExerciseWithFailingSamePoints() { Optional optional = scanExercise("failing_single_lib_same_point"); @@ -179,6 +259,10 @@ public void testScanExerciseWithNonexistentProject() { assertFalse(optional.isPresent()); } + private RunResult runTests(Path testExercise) { + return qmakePlugin.runTests(testExercise); + } + private RunResult.Status runTests(String testExercise) { return qmakePlugin.runTests(TestUtils.getPath(getClass(), testExercise)).status; } diff --git a/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/passing_multiple_lib_same_point.pro b/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/passing_multiple_lib_same_point.pro index 889a3ceca..17282d67a 100644 --- a/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/passing_multiple_lib_same_point.pro +++ b/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/passing_multiple_lib_same_point.pro @@ -1,12 +1,12 @@ TEMPLATE = subdirs SUBDIRS += \ - test_case_app \ - test_case_test_runner \ + src \ + test_case_test_runner \ test_case_lib \ test_case_lib2 - test_case_app.depends = test_case_lib \ - test_case_lib2 + src.depends = test_case_lib \ + test_case_lib2 test_case_test_runner.depends = test_case_lib \ test_case_lib2 diff --git a/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/test_case_app/main.cpp b/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/src/main.cpp similarity index 100% rename from tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/test_case_app/main.cpp rename to tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/src/main.cpp diff --git a/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/test_case_app/test_case_app.pro b/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/src/src.pro similarity index 100% rename from tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/test_case_app/test_case_app.pro rename to tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/src/src.pro diff --git a/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/test_case_test_runner/test_case_test_runner.cpp b/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/test_case_test_runner/test_case_test_runner.cpp index b4faff47f..f331d7e3f 100644 --- a/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/test_case_test_runner/test_case_test_runner.cpp +++ b/tmc-langs-qmake/src/test/resources/passing_multiple_lib_same_point/test_case_test_runner/test_case_test_runner.cpp @@ -15,7 +15,8 @@ void test_case_test_runner::test_function_one_here() { test_case_lib test_case; - POINT(test_function_one_here, 1); + POINT(test_function_one_here, testPoint1); + POINT(test_function_one_here, testPoint1.1); QVERIFY(!strcmp(test_case.piece_of_string(), "Hello, world!")); } @@ -24,8 +25,9 @@ void test_case_test_runner::test_function_two_here() { test_case_lib2 test_case; - POINT(test_function_two_here, 2); - QVERIFY(test_case.adding_ints(666, 1337) == 2003); + POINT(test_function_two_here, testPoint1); + POINT(test_function_two_here, testPoint1.2); + QVERIFY(test_case.adding_ints(666, 1337) == 2003); } @@ -33,6 +35,7 @@ void test_case_test_runner::test_function_two_here_2() { test_case_lib2 test_case; - POINT(test_function_two_here_2, 1); + POINT(test_function_two_here_2, testPoint1); + POINT(test_function_two_here_2, testPoint1.3); QVERIFY(test_case.adding_ints(-341, 428) == 87); }