Sql autorunner

[khsergvl]

What changes are you trying to make? (e.g. Adding or removing code, refactoring existing code, adding reports)

This pull request adds github workflow to run students queries and help to review assignments.
Result is truncated to up to 3 rows (github supports up to 65k symbols per comment). In case of failures it should not make any problem to students.

Was there another approach you were thinking about making? If so, what approach(es) were you thinking of?

It is built on the top of python unit test runner, and might be useful in case DSI will decide to implement strict unit test / test development driven assignment marking. I think we can change a trigger instead of on PR to on workflow_dispatch and trigger it manually during assignment review.

Were there any challenges? If so, what issue(s) did you face? How did you overcome it?

There are few limitations here, since runner right now executes only one sql statement within markers(query - end query).
We can collaborate to improve it, if it will be necessary.

How were these changes tested?

Please look here to see testing results:
https://github.com/khsergvl/sql/pulls
Two latest worse attention.

Other cohorts:

assignment sql files need markers to keep file parsed.

Checklist

• I can confirm that my changes are working as intended, *** any modification proposals are welcome ***
  With love to open-source

[mrpotatocode]

I have reviewed this conceptually but not at the code level.
It seems like a very sensible build and Sergii has formatted runner output in an easy-to-read form. I would like to see this merged, but since I have not used pytest, I'd greatly appreciate @danielrazavi and @RohanAlexander to give feedback!

[khsergvl]

@mrpotatocode
What you can do, you can fork repository to your personal account, merge the branch to your fork's main and try to submit a pr in the same way how students will do.
This is how it can be easy tested.
I can recommend to do it to every contributor / reviewer assigned.

[anjali-deshpande-hub]

Overall the code looks really good! There are a few of easy fixes. I have added inline comments.
I think these two are most important:

• Save errors to JSON (not just print) (line 78)
• Empty query (line 39)

Depending upon what we decide about error handling, we can fix review comment on line 20. We can choose to gracefully handle the error and always generate JSON file, for instance.

The rest of review comments are easy fixes.

[anjali-deshpande-hub]

@khsergvl Can you please look at

1. #anjali-deshpande-hub#1 In this PR, the assignment SQL file had some queries that are missing and Query 4 which has error; but the error shown by autorunner is incorrect.
2. #anjali-deshpande-hub#2 Here the assignment SQL file has correct answers, but the runner is failing. The file is running on DbBrowser. Maybe I am missing something. Please check.

[Dmytro-Bonislavskyi]

Hi @khsergvl after merging last changes we have an issue with: if (result.result.trim().length > 0) {
which we can change to if (result.result && result.result.length > 0) { to make it work for now.

Regarding " distinguish empty result set queries from non‑executed or failed ones." we can return something like this for both:
Queries 10 and 12:

End for errors (Q 2):

You can check my last PR in my repo to use this changes (If you want I can propose this changes to your repo and you can add them to next commit for current PR):
Dmytro-Bonislavskyi/sql_DC3#2

[khsergvl]

@Dmytro-Bonislavskyi well yeah, the condition of undefined was resolved. A spread of node js ...
About error handling I have something that was reported as a limitation. As far as I can remember fetch all doesn't work properly for multiple queries in the sql session. It means if the student will post i.e.:
Insert into ...; and After will do select * from fetchall will fail. In the same time queries would be valid.
This is the reason I decided to omit error handling and posting errors results ( they need to be trimmed also because of PR comment length limitation).

[khsergvl]

@anjali-deshpande-hub

[Dmytro-Bonislavskyi]

@khsergvl Sergii, Yes, I can see it with technically correct query with 2 statement like this (evaluating as error)

We can try to treat it as multi query in version 2 :).

And, we still can use "no rows returned" kind for "Empty-correct"/"just Empty" results to not just skip them silently.

[Dmytro-Bonislavskyi]

@khsergvl I add handling for multiple statements Queries in my repo, so now we can have it and also keep error handling as 3rd type. Give me other "limit cases", I can check them too.

[anjali-deshpande-hub]

@khsergvl @Dmytro-Bonislavskyi @mrpotatocode I have re-reviewed the code changes. It looks good to be merged now. Looking forward for this to be tested by our select testers.