Skip to content
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

exception handling Meta Tests #260

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AtillaColak
Copy link
Contributor

@AtillaColak AtillaColak commented Aug 1, 2023

I initially tried to work with compilation exceptions in the LibraryMetaTest but as there was already exception handling in RunMetaTestsStep I made the changes there. I was not sure about how to alert the teacher if there was a compilation exception so I worked with console logging. Please Let me know if something else was intended with "alerting the teacher".

Another thing to note is that when I made the changes in LibraryMetaTest as errors would already be handled here, in the RunMetaTestsStep we would get failing tests that were initially meant to be there for error handling. So this was another reason for why I worked with RunMetaTestsStep folder instead.

I did not delete the other changes, just in case, only commented them out. So when we reach a final decision on where to handle errors, I will delete those commented lines.

Closes #227

@mauricioaniche
Copy link
Contributor

Hi, @AtillaColak, this is a hard MR to review, as I can't compare the previous code with the new one. I think it's easier if you don't have all the comments there, so that git diff can do its job.

What exactly are you trying to achieve? Which issue is this about?

@AtillaColak
Copy link
Contributor Author

Hi, @AtillaColak, this is a hard MR to review, as I can't compare the previous code with the new one. I think it's easier if you don't have all the comments there, so that git diff can do its job.

What exactly are you trying to achieve? Which issue is this about?

This is an MR for the issue number #227. I've removed the unnecessary comments so it should be easier to review now.

Copy link
Collaborator

@martinmladenov martinmladenov left a comment

Choose a reason for hiding this comment

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

System.out and System.err are not reported by Andy; a good way to inform the teacher would be to use GenericFailures with an appropriate exception message (result.genericFailure(this, ...)).

Could you also write a test to verify that your changes work as expected?

@AtillaColak
Copy link
Contributor Author

sure I'll get to it tonight

@AtillaColak
Copy link
Contributor Author

@martinmladenov Hey Martin could you take a look at this? For some reason, the test I wrote for the exam scenario fails and returns false for the "there is genericfailure" check. I would not want to disturb you if I could have solved it myself but I tried for some hours and was still at a dead end. Maybe you have an idea of what I could be obviously missing, so that I can fix it.

@martinmladenov
Copy link
Collaborator

@AtillaColak
Are you sure that your changes work correctly? If I'm not mistaken, in the current production version, if there's a compilation error due to a badly written meta test, Andy considers that meta test to fail (i.e. there's no generic failure raised). Your changes add an extra condition for exam mode in the part that raises a generic failure, but I don't think this part of the code gets triggered when a meta test encounters a compilation error.

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.

If there's a compilation error in a meta test, inform the teacher
3 participants