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.

anjali-deshpande-hub · 2026-03-25T20:59:38Z

tests/test_assignment.py

+            print(f"An unexpected error occurred: {e}")
+    json.dump(test_result, json_file, indent=2)
+    json_file.close()
+    assert True,  "test execution query {} result {}".format(queries, test_result)


I am not sure if this Assert True statement is of any use. I think it can be removed.

The purpose of it to have report in the future in case DSI will want to go with unit testing style.

In that case, we should probably comment the line and add a comment about the intent. What do you think?

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

anjali-deshpande-hub · 2026-03-25T22:18:57Z

tests/test_assignment.py

+    run_assignment(sqlite_db, file_path)
+
+def run_assignment(sqlite_db, file_path):
+    json_file = open("test-results.json", "w")


Just a suggestion to use with open("..."). It will ensure that the file is always properly closed, even if an exception occurs before close()

Well yeah, it normally it make sense to close streams.

anjali-deshpande-hub · 2026-03-25T22:21:09Z

.github/workflows/sql_assignment_runner.yml

+        run: |
+          pytest tests/test_assignment.py --file_path="${{steps.changes.outputs.assignment_changed}}" --tb=short --disable-warnings \
+                 --junitxml=pytest-report.xml || true
+


I am not sure if this option --junitxml=pytest-report.xml is needed? pytest-report.xml doesn't seem to be created.

The purpose of it to have report in the future in case DSI will want to go with unit testing style.

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.

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

		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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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