test: say what the run membership case in RemoveStudentRunControllerTest asserts - #346
Open
Isaries wants to merge 1 commit into
Open
Conversation
The test was named removeStudent_StudentNotInRun_RemoveStudentFromRun, but APIControllerTest.createWorkgroups builds workgroup2 under run1 with student2 as its member, so student2 is in the run. What the fixture does model is the state the doc comment names as the reason not to reject: in a workgroup, no longer in the period, since run1Period1 holds only student1. The comment also claimed the test showed both steps of removeStudentFromRun are scoped to the run. studentService is mocked here, so nothing about that is exercised and the test would pass identically for a fully enrolled student. It asserts the controller's delegation and the comment now says so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #333, which is merged. No production code changes.
removeStudent_StudentNotInRun_RemoveStudentFromRundoes not set up the state its name describes.APIControllerTest.createWorkgroups()buildsso student2 is a member of a workgroup belonging to run1. What the fixture does model is the state the doc comment names as the reason not to reject: still in a workgroup, no longer in a period, since
run1Period1's own member list holds only student1. Renamed toremoveStudent_StudentInWorkgroupButNotInPeriod_RemoveStudentFromRun.The doc comment also claimed the test showed that both steps of
removeStudentFromRunare scoped to the run.studentServiceis mocked here, so nothing about that is exercised and the test would pass identically for a fully enrolled student. What it does assert is that this controller adds no run membership check of its own, which is a deliberate decision worth keeping locked in, and the comment now says that and nothing more.The comment is explicit that the role check is the only thing asked about the target user, and points at
removeStudent_NoWritePermission_ThrowAccessDeniedfor the write-permission half, so that "the only check" cannot be read as the endpoint being unguarded against a teacher acting on someone else's run.