search

SERG-Delft / andy

Andy assesses student's test code. It's used in CSE1110, TU Delft.
MIT License
78 stars 22 forks source link

Improve how RunConfiguration is set in the meta test #175

Open mauricioaniche opened 1 year ago

mauricioaniche commented 1 year ago

Currently no RunConfiguration is set in the meta test. Each meta test then tries to find one, an exception happens, and we inject a default one.

First of all, there's no need for a default run configuration. If there's none, we should not even start. Second, we can skip this step then, when running meta tests.

FrancescoHamar commented 1 year ago

Hello! I would like to work on this issue with Tomasz Puczel (@puctom)

mauricioaniche commented 1 year ago

Great! @puctom, post something here so I can assign you too!

puctom commented 1 year ago

i would like to work on this too :)

mauricioaniche commented 1 year ago

I can't remember precisely what I meant with the description of this ticket, I'll try to dive a bit more. But if you debug the execution of a meta test, you'll see the exception when it comes to finding the Configuration.

FrancescoHamar commented 1 year ago

Hello! We have a doubt about this issue. You mention "Currently no RunConfiguration is set in the meta test. Each meta test then tries to find one, an exception happens, and we inject a default one". However, we believe that the run configuration is handled by the context and when the RunMetaTestsStep happens, if a default RunConfiguration is used, there won't be any meta tests in the first place. Could you help clear up a bit what you meant by the exception that we should avoid?

mauricioaniche commented 1 year ago

IIRC, what I meant was that, when we are running a meta test, and then its execution gets to the GetRunConfigurationStep, a NoSuchElementException happens and then line 31 is executed, setting a default configuration. As a way to confirm that, put a debug point on line 31 (the line inside the catch block) and see if it ever gets executed.

If it does get executed, I would like to avoid that. It makes no sense to use a try/catch to handle a flow that we expect to happen. Somehow and somewhere we need to do "if this is a meta test, then, new DefaultRunConfiguration`.

(There might be a chance that this line isn't executed anymore, as we have made many changes since I opened up this ticket! If so, we close it!)

Let me know if that clarifies the issue!