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

Handle case where no test groups are runnable by students #7003

Merged

Conversation

mimischly7
Copy link
Contributor

@mimischly7 mimischly7 commented Mar 29, 2024

Motivation and Context

In the autotest-settings page (in the instructor view), the instructor has the ability to make all test groups NOT-runnable by students. Currently, when a student presses the "Run Tests" button, a request is still sent to the auto-testing server and 0/0 is shown on the website. We want to change this so that, if no test-groups are student-runnable, we avoid the unnecessary request to the autotest server and present an appropriate message on the screen.

Screenshot 2024-03-29 at 2 18 56 AM

Your Changes

Description:
An Assignment object has a test_groups association (corresponding to a test_groups table in the postgres database) holding information about the autotest-settings set by the instructor for that assignment, including who can run the tests, which is an array equal to one of the four following: ["instructor", "student"], ["instructor"], ["student"], [].
In the execute_test_run action of the automated_tests_controller, we examine the test_groups attribute of the current assignment and if there is no test-group runnable by a student, we flash an appropriate message and return early.

Type of change (select all that apply):

  • Bug fix (non-breaking change which fixes an issue)

Testing

I had to modify the current tests because this pull request does change the behavior of automated testing. In particular, if none of the test-groups are runnable by students, now the test-execution job does not get enqueued (which was the whole point, since if it gets enqueued to AutotestRunJob then an unnecessary request will be made to the autotest server). But this made some of the existing tests for execute_test_run (in automated_test_controller.rb) fail. This is because the factory-produced assignment object used in these tests had the test_groups attribute/association uninitialized (due to how an :assignment is spawned by a factory). To test the new behavior, instead of generating a simple :assignment, the tests now use an :assignment_with_criteria_and_test_results, and in the tests with context "when the student is allowed to run tests", I had to manually update the test_groups attribute of the assignment to specify that the test_group is runnable by students. Overall, we are now testing the case where none of the test groups are runnable by students, so no job should be enqueued, and the case where at least one test-group is student-runnable, in which case the job should be enqueued as usual.

Questions and Comments (if applicable)

Checklist

  • I have performed a self-review of my own code.
  • I have verified that the pre-commit.ci checks have passed.
  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported on Coveralls.
  • I have added tests for my changes.
  • I have updated the Changelog.md file.

In the autotest-settings page (in the instructor view), the instructor has the ability to make all test groups NOT-runnable by students. Currently, a request is still sent to the autotesting server and 0/0 is shown on the website. Instead, we check this inside the automated_tests_controller#execute_test_run action, and if NO test groups are student-runnable, we flash an appropriate messgae and return, avoiding a redundant request to the autotesting server.
I had to modify the current tests because the change of this pull request does change the behavior of automated testing. In particular, if none of the test-groups are runnable by students, now the test-execution job does not get enqeued as a job (which was the whole point, since if it gets enqueued to AutotestRunJob then an unnecessary request will be made to the autotest server). But this made some of the existing tests for execute_test_run (in automated_test_controller.rb) fail. This is because the factory-produced assignment object used in these tests had the test_groups attribute/association uninitialized. To test the new behavior, instead of generating a simple :assignment, the tests now use an :assignment_with_criteria_and_test_results, and in the tests with context , I had to manually update the test_groups attribute of the assignment to specify the test_group is runnable by students. Overall, we are now testing the case where none of the test groups are runnable by students, so no job should be enqueued, and the case where at least one test-group is student-runnable, in which case the job should be enqueued as usual.
@mimischly7 mimischly7 requested a review from david-yz-liu March 29, 2024 06:42
Copy link
Collaborator

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

@mimischly7 good work, in addition to the comments I left, make the equivalent change for instructor-run tests.

app/controllers/automated_tests_controller.rb Outdated Show resolved Hide resolved
config/locales/views/automated_tests/en.yml Outdated Show resolved Hide resolved
spec/controllers/automated_tests_controller_spec.rb Outdated Show resolved Hide resolved
Refactor the new testing by creating new factories for assignments with/without student-runnable tests (and, by extension, corresponding test-group factories) and use  to improve query efficiency.
…7/Markus into autotest-task-student-runnable

merge-commit
@coveralls
Copy link
Collaborator

coveralls commented Mar 30, 2024

Pull Request Test Coverage Report for Build 8573068288

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 29 of 29 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 91.917%

Totals Coverage Status
Change from base Build 8498218632: 0.3%
Covered Lines: 41077
Relevant Lines: 43986

💛 - Coveralls

spec/controllers/automated_tests_controller_spec.rb Outdated Show resolved Hide resolved
app/controllers/automated_tests_controller.rb Outdated Show resolved Hide resolved
app/controllers/automated_tests_controller.rb Outdated Show resolved Hide resolved
app/controllers/automated_tests_controller.rb Outdated Show resolved Hide resolved
Imrpove test-group retrieval using pluck (both active record pluck and enumerable pluck) and remove redundant declarations from the rspec tests
…7/Markus into autotest-task-student-runnable

merge-with-origin
@mimischly7 mimischly7 requested a review from david-yz-liu April 5, 2024 18:04
@david-yz-liu david-yz-liu merged commit 7059ba4 into MarkUsProject:master Apr 9, 2024
6 checks passed
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.

3 participants