ls1intum / Ares

The Artemis Java Test Sandbox. A JUnit 5 Extension for Easy and Secure Artemis Java Testing
https://ls1intum.github.io/Ares/
MIT License
18 stars 7 forks source link

Improve AST functionality with excludeMain() #369

Open sarpsahinalp opened 2 months ago

sarpsahinalp commented 2 months ago

Improved the current AST functionality to allow instructors to exclude the Main method from AST checks.

MaisiKoleni commented 2 months ago

Why exclude main? Could you describe a use case here? Why not any other method? Or just just specific issues?

This potentially weekend security (or maybe better called reliability, accuracy here). What are the thoughts on that and the rationale?

Strohgelaender commented 2 months ago

Why exclude main? Could you describe a use case here?

I believe the idea here is to allow students restriction-free local testing of their own solution using the main method.

But I'm not sure if just excluding the main method is the best approach for this. Consider a student submission like this:

 public static void main(String[] args) {
    testCase1();
    testCase2();
 }

Even though the testCasex() methods are clearly only for local testing and have nothing to do with the exercise, they would be included in the analysis. We probably need to implement some kind of dependency tree to distinguish methods (only) used by the entry point(s) and methods used in the solution of the exercise.

Edit: I just noticed that your other PR implements a MethodCallGraph class. I did not find time to look at this PR in detail, but maybe you can combine these approaches.

MaisiKoleni commented 2 months ago

If we are going in that direction, a boolean flag is not a sustainable solution (this is quite common). Consider a design that is more flexible and sustainable. We can still offer a sensible selection of implementations in some way. Think of collect and Collectors, Charset and StandardCharsets, and so on, or similar APIs.

sarpsahinalp commented 1 month ago

If we are going in that direction, a boolean flag is not a sustainable solution (this is quite common). Consider a design that is more flexible and sustainable. We can still offer a sensible selection of implementations in some way. Think of collect and Collectors, Charset and StandardCharsets, and so on, or similar APIs.

I will take a look in to those APIs thank you for your review!

sarpsahinalp commented 1 month ago

Why exclude main? Could you describe a use case here?

I believe the idea here is to allow students restriction-free local testing of their own solution using the main method.

But I'm not sure if just excluding the main method is the best approach for this. Consider a student submission like this:

public static void main(String[] args) {
   testCase1();
   testCase2();
}

Even though the testCasex() methods are clearly only for local testing and have nothing to do with the exercise, they would be included in the analysis. We probably need to implement some kind of dependency tree to distinguish methods (only) used by the entry point(s) and methods used in the solution of the exercise.

Edit: I just noticed that your other PR implements a MethodCallGraph class. I did not find time to look at this PR in detail, but maybe you can combine these approaches.

For this we need to use the MethodCallGraph which is again not in scope of this PR. Will take this to Markus further!