Sql autorunner by khsergvl · Pull Request #398 · UofT-DSI/sql

khsergvl · 2026-03-24T21:14:34Z

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

Sql autorunner

Change branch filtering

Fix path extraction

mrpotatocode · 2026-03-24T21:44:14Z

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 · 2026-03-25T04:12:34Z

@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.

tests/test_assignment.py

anjali-deshpande-hub · 2026-03-25T21:11:25Z

tests/test_assignment.py

+            rows = run_query(sqlite_db, parsed_query['query'])
+            test_result.append( { "number": parsed_query['number'], "query": parsed_query['query'], "result": rows[0:3] })
+        except Exception as e:
+            print(f"An unexpected error occurred: {e}")


Instead of this print, how about we store the error in the test-results.json file like how we store the result in the json file. We can do something like -
test_result.append({"number": parsed_query['number'], "query": parsed_query['query'], "error": str(e)})

and then parse "error" tag in the JSON file to output "Error". The reason I am saying this is because the print swallows the error and doesn't output anywhere.

See test case anjali-deshpande-hub#1, I have Query 4 which has "FROM" clause missing (incorrect syntax) and the autorunner chooses to skip Query 4. Only Query 3,5 are shown.

I think its better to show the Query and may be under "Results", show "Error"

I'm was not sure what is the best approach here, since it doesn't gives an explanation not to LS not too students what to do? In the same time I'm aware about possible case when multiple sql queries will go as one task and fetchall() might fail, but queries still valid. I.e. insert https://github.com/UofT-DSI/sql/blob/main/03_instructional_team/DC_Cohort_Rubrics/LS_Assignment2_rubric.md

anjali-deshpande-hub · 2026-03-25T22:11:32Z

tests/test_assignment.py

+        if stripped.lower().startswith("--query"):
+            if current_query is not None:
+                raise AssertionError("Nested QUERY blocks are not allowed")
+


The Assertions in this function will result in the autorunner to fail. I understand that in these cases, the learning support can see the error messages under 'Run tests' section of the workflow? See test case anjali-deshpande-hub#2
Depending upon what we decide to about error handling, this can change.

anjali-deshpande-hub · 2026-03-25T22:14:06Z

tests/test_assignment.py

+                continue  # ignore stray END
+
+            query = "\n".join(buffer).strip().rstrip(";")
+


I think somewhere here we should code for missing query. The query markers are present, but the code inside is missing. See anjali-deshpande-hub#1, Currently I am seeing:

✅ Query 1: **, Results: No data

Instead, maybe we can say:

✅ Query 1: No query

tests/test_assignment.py

.github/workflows/sql_assignment_runner.yml

anjali-deshpande-hub · 2026-03-25T22:29:28Z

tests/conftest.py

+
+def pytest_addoption(parser):
+    parser.addoption("--file_path", action="store", default="02_activities/assignments/DC_Cohort/assignment1.sql")
+


We can choose to save these filepaths in environment variable (default="02_activities/assignments/DC_Cohort/assignment1.sql", "05_src/sql/farmersmarket.db"). Maybe under "env" key in .yml file

The intent to provide a file name as parameter here, coming from the point, that we have multiple cohort running in parallel. It can be used for Micro credentials, etc. So default is basically default. In the same time this path is extracted from the file list modified in the pull request.

anjali-deshpande-hub

@khsergvl Can you please look at

#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.
#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.

anjali-deshpande-hub · 2026-03-29T23:19:11Z

.github/workflows/sql_assignment_runner.yml

-                body +=  `${table} \n`
-                body += `\n`
-                body += `-------------------------------------------------------- \n` 
+                if (result.result.trim().length > 0) {


I think we have a problem with new if statement. I am not sure why this was added. Empty result sets are skipped entirely. There is no way to distinguish empty result set queries from non‑executed or failed ones.

Dmytro-Bonislavskyi · 2026-03-30T17:51:14Z

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 · 2026-03-30T19:00:07Z

@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 · 2026-03-30T19:00:29Z

@anjali-deshpande-hub

Dmytro-Bonislavskyi · 2026-03-30T19:18:14Z

@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 · 2026-03-30T19:45:55Z

@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.

khsergvl added 9 commits March 24, 2026 13:50

Implement sql queries runner

cf333a0

Implement sql queries runner

3eda95a

Merge pull request #10 from khsergvl/sql-autorunner

9089fe8

Sql autorunner

Change branch filtering

f842ff4

Merge pull request #12 from khsergvl/sql-autorunner

5fd7625

Change branch filtering

Fix path extraction

aa79e26

Merge pull request #14 from khsergvl/sql-autorunner

78001ed

Fix path extraction

Comments fix

2843051

Minor modification

4e74f54

khsergvl assigned RohanAlexander, danielrazavi, mrpotatocode, Dmytro-Bonislavskyi and anjali-deshpande-hub Mar 24, 2026

anjali-deshpande-hub requested changes Mar 25, 2026

View reviewed changes

PR addressed

0349f73

anjali-deshpande-hub requested changes Mar 29, 2026

View reviewed changes

PR addressed 1

4ee1854

		continue # ignore stray END

		query = "\n".join(buffer).strip().rstrip(";")


		def pytest_addoption(parser):
		parser.addoption("--file_path", action="store", default="02_activities/assignments/DC_Cohort/assignment1.sql")

Conversation

khsergvl commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

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

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

How were these changes tested?

Other cohorts:

Checklist

Uh oh!

mrpotatocode commented Mar 24, 2026

Uh oh!

khsergvl commented Mar 25, 2026

Uh oh!

anjali-deshpande-hub left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anjali-deshpande-hub Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

khsergvl Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

anjali-deshpande-hub Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

anjali-deshpande-hub Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anjali-deshpande-hub Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

khsergvl Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

anjali-deshpande-hub left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjali-deshpande-hub Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Dmytro-Bonislavskyi commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khsergvl commented Mar 30, 2026

Uh oh!

khsergvl commented Mar 30, 2026

Uh oh!

Dmytro-Bonislavskyi commented Mar 30, 2026

Uh oh!

Dmytro-Bonislavskyi commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

khsergvl commented Mar 24, 2026 •

edited

Loading

anjali-deshpande-hub left a comment •

edited

Loading

anjali-deshpande-hub left a comment •

edited

Loading

Dmytro-Bonislavskyi commented Mar 30, 2026 •

edited

Loading