edulinq / autograder-py

The Python interface to the autograder.
MIT License
4 stars 7 forks source link

Added the ability to request a removal of a specified submission to the CLI #7

Closed OliverLok closed 8 months ago

OliverLok commented 8 months ago

The code here looks good! Only comment on the existing code is that you should rename: "remove-submission" -> "remove". Since it is already in the "submission" package, the prefix is redundant.

The only remaining thing here is tests.

API tests are done with JSON data: https://github.com/eriq-augustine/autograder-py/tree/main/tests/api/data The testing infrastructure knows how to look through the data, find the tests, call the right API method, and check the results. The easiest way to get the expected output for the tests is to run the CLI call on the Python side (e.g. python3 -m autograder.cli.submission.remove --verbose ...). With the verbose flag, the exact request and response will be printed out.

You can see examples with peek: https://github.com/eriq-augustine/autograder-py/blob/main/tests/api/data/test_submission_peek_other_specific.json

Note that the data for API tests will actually be validated later (automatically) in the CI for the server.

With that logic, should "removesubmission" in api/submission also be renamed to remove? Also, just to clarify, is the CI sending requests and comparing the request/response JSON to the corresponding json file in tests/api/data? I'm only asking because when I changed found-user to true in test_submission_peek_other_missing.json (where the target-email was "ZZZ"), ./test.sh still said No issues found.

eriq-augustine commented 8 months ago

With that logic, should "removesubmission" in api/submission also be renamed to remove?

Yes.

Also, just to clarify, is the CI sending requests and comparing the request/response JSON to the corresponding json file in tests/api/data?

The API tests (which is run in the CI) is sending itself HTTP requests and checking that they match up against it's own expected output (the same file is used to specify the input an output): https://github.com/eriq-augustine/autograder-py/blob/main/tests/api/test_api.py#L28

I'm only asking because when I changed found-user to true in test_submission_peek_other_missing.json (where the target-email was "ZZZ"), ./test.sh still said No issues found.

https://github.com/eriq-augustine/autograder-py/blob/main/tests/api/test_api.py#L32 On this side, the data is expected to be correct. So when you changed the data, you changed what the test thought was correct. But the server will verify the data in it's own CI: https://github.com/eriq-augustine/autograder-server/blob/main/.ci/verify-py-test-data.sh

eriq-augustine commented 8 months ago

Looks like I spoke too soon, CI is failing. Did you run tests locally?