Skip to content

Cluster haskell syntax errors using regex #3

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.

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

Open
wants to merge 37 commits into
base: commonerrors
Choose a base branch
from

Conversation

C8R15T14N
Copy link

  • New testtypes "Haskell Syntax Test" and "Haskell Runtime Test"
  • Haskell Syntax Test: ready to use. Clusters syntax errors of haskell submissions using a regex-based approach.
  • Haskell Runtime Test: not yet implemented (is currently very similar to the already implemented DockerTest)

C8R15T14N and others added 16 commits April 21, 2025 14:02
- implementation missing, marked with TODO@CHW and will be similar as in docker test
- view: HaskellRuntimeTestManagerView (based on DockerTestManagerOverView)
- controller: HaskellRuntimeTestManager (based on DockerTestManager)
Conflicts during rebase:
	src/main/java/de/tuclausthal/submissioninterface/persistence/dao/TestDAOIf.java
	src/main/java/de/tuclausthal/submissioninterface/persistence/dao/impl/TestDAO.java
	src/main/java/de/tuclausthal/submissioninterface/servlets/controller/TestManager.java
@csware
Copy link
Owner

csware commented Apr 21, 2025

Wouldn't an extension/property to the DockerTest be a better approach to not duplicate a lot of code?

- DockerTestManager now also handles case of HaskellRuntimeTest (since HaskellRuntimeTest is a specialization of DockerTest)
- HaskellRuntimeTestManager is kept for consistency (it is the corresponding controller servlet of HaskellRuntimeTestManagerView)
@C8R15T14N
Copy link
Author

Wouldn't an extension/property to the DockerTest be a better approach to not duplicate a lot of code?

Regarding the Haskell Runtime Test:

  • The view servlet HaskellRuntimeTestManagerView currently contains duplicated code. I'm already working on automating the test case definition process, which involves rewriting most of this servlet. These changes are not included in this pull request.
  • The duplicated code in the corresponding controller servlet HaskellRuntimeTestManager can indeed be avoided (new commit b9865a1).
  • In all other places (persistence/datamodel/HaskellRuntimeTest and testframework/tests/impl/HaskellRuntimeTest), HaskellRuntimeTest already inherits from DockerTest. I agree, that the Haskell Runtime Test should extend the existing functionality of the Docker Test.

@esat553
Copy link

esat553 commented Apr 24, 2025

Wouldn't an extension/property to the DockerTest be a better approach to not duplicate a lot of code?

Regarding the Haskell Syntax Test:

HaskellSyntaxTest now extends the a modularaized version of DockerTest and thereby uses already defined functions hopefully this will reduce code duplicates.

@csware
Copy link
Owner

csware commented Apr 25, 2025

I was curious whether a new attribute for the dockertest may already solve the issue. Such attribute could later also be used to selected different images.

@C8R15T14N
Copy link
Author

I was curious whether a new attribute for the dockertest may already solve the issue. Such attribute could later also be used to selected different images.

We decided to implement the HaskellSyntaxTest and HaskellRuntimeTest as independent test types (inheriting from the DockerTest) instead of adding an attribute to the DockerTest for the following reasons:

  1. Consistent user experience and implementation: In the case of Java, the syntax test, IO test, and JUnit test are implemented as separate test types. To maintain consistency in both - the user experience and the structure of GATE - we created separate syntax and runtime tests for Haskell as well, instead of adding the whole functionality inside of the DockerTest.

  2. Avoiding to over-complicate the DockerTest: Implementing the HaskellSyntaxTest and HaskellRuntimeTest inside of the DockerTest using a new attribute would require

    • integrating the entire logic of both tests inside of the DockerTest
    • adding case distinctions to choose the test logic and user interface according to the testtype stored in the attribute.

    To prevent the DockerTest from becoming overly complex, we decided to use Java inheritance to avoid manual case distinctions and to separate the implementation of the different testtypes into different classes, while reusing the inherited functionality of the DockerTest to minimise duplicated code.
    Since both tests are being developed as part of two separate bachelor theses, they will become more complex in the next few weeks.

  3. Versatility of the DockerTest:
    The DockerTest can be used for other programming languages as well (given that all required packages are installed in the image), since it allows the user to manually specify arbitrary testing expressions. We therefore implemented our Haskell specific extensions to the DockerTest in subclasses, which keeps the DockerTest versatile and language-independent. This way, adding further DockerTest based tests that are specifically adapted to other programming languages, is as easy as creating a new subclass and extending the generic functionality of the DockerTest.

  4. Strictly separated implementations: These are two separate bachelor theses - Esat is working on the HaskellSyntaxTest while I'm implementing the HaskellRuntimeTest. It would therefore be desirable to separate our implementations as clearly as possible.

@C8R15T14N C8R15T14N marked this pull request as ready for review April 25, 2025 18:42
@csware
Copy link
Owner

csware commented May 7, 2025

Please make sure all files have a new line at the end.

Copy link
Owner

@csware csware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is fine for the prototype. Please use final as much as possible.


public RegexBasedHaskellClustering(Session session) {
this.session = session;
this.clusters = new LinkedHashMap<>();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be done in a static way?


@Override
public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
getServletContext().getNamedDispatcher(DockerTestManager.class.getSimpleName()).forward(request, response);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to be good style to just forward the calls to the DockerTestManager, i.e. this servlet is not necessary.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having different views for the HaskellTest is okay.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to be good style to just forward the calls to the DockerTestManager, i.e. this servlet is not necessary.

The HaskellRuntimeTestManager controller servlet requires some additional functionality that is not contained in the DockerTestManager controller servlet (this is still work in progress). My idea was to handle all actions that are specific to the haskell runtime test inside of the HaskellRuntimeTestManager, and forward all other calls to the DockerTestManager.

As an example, I just implemented a new "generateNewTestSteps" action inside the HaskellRuntimeTestManager, that automatically generates a certain number of haskell runtime testcases. All other actions are forwarded to the DockerTestManager (see new commit 402f094).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it should be added as soon as it is required.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it should be added as soon as it is required.

My choice of words may have been misleading - by saying "as an example", I didn’t mean that the "generateNewTestSteps" action is optional or merely illustrative for my idea. It is definitely required, since automatically generating haskell testcases is a major part of my bachelor thesis.

What I meant is that I expect to add more actions in the future that follow the same pattern - in that sense, "generateNewTestSteps" serves "as an example" for them.

JsonObject testOutputJson = Json.createReader(new StringReader(testResult.getTestOutput())).readObject();
String stderr = testOutputJson.containsKey("stderr") ? testOutputJson.getString("stderr") : "";

String keyStr = "HaskellSyntax: ";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the "Haskell" prefix really needed?


import de.tuclausthal.submissioninterface.persistence.datamodel.TestResult;

public interface HaskellErrorClassifierIf {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats is the advantage of this interface?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to have more Classes for a Static code analysis e.g. k-means or clustering by AST but by doing the clustering statically right now the interface is obsolete

private Path tempDir;
protected static final Random random = new Random();
protected final String separator;
protected Path tempDir;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protected allows more than you think, i.e. protected allows field access from within the same package.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't had that on mind. Thank you for the hint. I fixed it in the upcoming version

return testCode.toString();
}

protected void debugLog(List<String> params, Path studentDir){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this method necessary?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a way to modulize the logging for individual logging messages in subclasses but it is currently not in use so I will remove it

Comment on lines 176 to 178
response.sendRedirect(Util.generateRedirectURL(TaskManager.class.getSimpleName()
+ "?action=editTask&lecture=" + task.getTaskGroup().getLecture().getId()
+ "&taskid=" + task.getTaskid(), response));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are lecture, taskid and testid necessary?

@csware
Copy link
Owner

csware commented May 13, 2025

Overall the formatting is not correct, also the order of imports.


public HaskellSyntaxTest(de.tuclausthal.submissioninterface.persistence.datamodel.HaskellSyntaxTest test) {
super(test);
separator = "#<GATE@" + random.nextLong() + "#@>#";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why necessary?

set -e
echo '%s'
for file in *.hs; do
ghci -ignore-dot-ghci -v0 -ferror-spans -fdiagnostics-color=never -Wall -e ":load $file" -e ":quit"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this properly escaped?

return entry.getKey();
}
}
return "Sonstige Fehler";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo?

@csware
Copy link
Owner

csware commented May 13, 2025

There are warnings in the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants