CSC/ECE 517 Fall 2019 - E1969. Fixes for reviews not being available
About Expertiza
Expertiza is an open source project based on Ruby on Rails framework. It is a platform which allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for and supports project bidding activity. Students can form teams on Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.
Introduction
In the project, we address two issues. One of the issues is expected to be non-existent, therefore in order to confirm that, we write a set containing maximum possible test paths which may reproduce the issue. It turns out that after running the entire test suite containing a total of 13 distinct possible paths to arrive at error-prone position, the bug is not reproduced. Therefore, we conclude that the bug does not exist. In the second issue, we make necessary changes in the code and fix the system. Following that, we test the updated system as well.
Motivation
Several issues prevail in the current Expertiza deployment which may be inconspicuous but are fatal while considering functioning of such a critical application. Therefore, in an attempt to make the system flawless, we try to resolve two issues, Issue #1321 and Issue #1211.
Problem Statement
The purpose of this project was two-fold.
- First, for Issue #1321, we were tasked with investigating whether or not a bug exists that can prevent users from being able to start a review. If the bug exists, we were to fix it; if not, we were to create an automated test suite to provide evidence of the bug's absence.
- Second, for Issue #1211, we were tasked with fixing a bug that prevented reviews from being completed during resubmission periods. Further, test cases had to be written to show that this bug was fixed.
Issue #1321
Reported Problem
It was reported by a student that an assignment was not able to be reviewed even though it was during the review period. This bug was thought to be fixed in the past when Issue #915 was merged. However, in December of 2018, another student reported this issue occurring again.
Investigation Approach
It is suspected that if this bug exists, it is caused by an abnormal user flow path. That is, caused by a user navigating to the point where they should be able to conduct a review, but doing so in an indirect way. To begin our investigation of this bug, a flow chart showing most plausible user flow paths was created. We then used this flow chart to guide our investigation efforts. This flow chart is found below:
From this point, we began to manually test the user flow paths represented in our flow chart on our local development environment. Doing this revealed no evidence that the bug exists. We then set out to create an automated test suite that would test the user flow paths in a more quick and reliable way. The code for these tests, as well as the test cases they cover are produced in the testing section below.
Test Plan
The purpose of the following test is to iterate over a set of plausible user flow paths that terminate in being able to begin a review. The following flow paths are tested, where each step in the path is either a user clicking in the UI or navigating to a page directly using their browser:
Clicking around in UI like normal
- Assignments -> ReviewTestAssignment -> Others' work -> Request a new submission to review -> Begin
- Contact Us -> Assignments -> ReviewTestAssignment -> Others' work -> Request a new submission to review -> Begin
- Home -> Assignments -> ReviewTestAssignment -> Others' work -> Request a new submission to review -> Begin
- Profile -> Assignments -> ReviewTestAssignment -> Others' work -> Request a new submission to review -> Begin
Going from different pages directly to the specific assignment page using address bar
- Assignments -> /student_task/view?id=3 -> Others' work -> Request a new submission to review -> Begin
- Contact Us -> /student_task/view?id=3 -> ReviewTestAssignment -> Others' work -> Request a new submission to review -> Begin
- Home -> /student_task/view?id=3 -> ReviewTestAssignment -> Others' work -> Request a new submission to review -> Begin
- Profile -> /student_task/view?id=3 -> ReviewTestAssignment -> Others' work -> Request a new submission to review -> Begin
Going from different pages directly to the specific assignment review page using address bar
- Assignments -> /student_review/list?id=3 -> Request a new submission to review -> Begin
- Contact Us-> /student_review/list?id=3 -> Request a new submission to review -> Begin
- Home -> /student_review/list?id=3 -> Request a new submission to review -> Begin
- Profile -> /student_review/list?id=3 -> Request a new submission to review -> Begin
Going from assignment list to specific assignment page, back to assignment list and then finishing review
- student_task/list -> ReviewTestAssignment -> /student_task/list -> ReviewTestAssignment -> Others' work -> Request a new submission to review -> Begin
These test cases are implemented using the following code. This code is located in /spec/features/peer_review_spec.rb as this file handles the testing of reviews. In an attempt to keep the code DRY, we loop over the test cases contained in a large array. This array consists of smaller arrays, each representing a user flow path. Each test case array also contains smaller arrays which are the steps in this user flow path. The first element in these "step" arrays are the name of the button/link the browser should click/visit. The second element is what should be present on the page once the navigation is complete. This approach not only keeps the code DRY, but makes it trivial to add further test cases in the future, should the need arise.
#participant id is 3 for student3
normal_test_paths = [
[["Student_task","Select an assignment"] ,["ReviewTestAssignment","Others' work"], ["Others' work",'Reviews for "ReviewTestAssignment"']],
[["Contact Us","Welcome!"], ["Student_task","Select an assignment"], ["ReviewTestAssignment","Others' work"], ["Others' work",'Reviews for "ReviewTestAssignment"']],
[["Home","Welcome!"], ["Student_task","Select an assignment"], ["ReviewTestAssignment","Others' work"], ["Others' work",'Reviews for "ReviewTestAssignment"']],
[["Profile","User Profile Information"], ["Student_task","Select an assignment"], ["ReviewTestAssignment","Others' work"], ["Others' work",'Reviews for "ReviewTestAssignment"']],
[["Student_task","Select an assignment"], ["/student_task/view?id=3","Others' work"], ["Others' work",'Reviews for "ReviewTestAssignment"']],
[["Contact Us","Welcome!"], ["/student_task/view?id=3","Others' work"], ["Others' work",'Reviews for "ReviewTestAssignment"']],
[["Home","Welcome!"], ["/student_task/view?id=3","Others' work"], ["Others' work",'Reviews for "ReviewTestAssignment"']],
[["Profile","User Profile Information"], ["/student_task/view?id=3","Others' work"], ["Others' work",'Reviews for "ReviewTestAssignment"']],
[["Student_task","Select an assignment"], ["/student_review/list?id=3",'Reviews for "ReviewTestAssignment"']],
[["Contact Us","Welcome!"], ["/student_review/list?id=3",'Reviews for "ReviewTestAssignment"']],
[["Home","Welcome!"], ["/student_review/list?id=3",'Reviews for "ReviewTestAssignment"']],
[["Profile","User Profile Information"], ["/student_review/list?id=3",'Reviews for "ReviewTestAssignment"']],
[["/student_task/list","Select an assignment"], ["ReviewTestAssignment","Others' work"], ["/student_task/list","Select an assignment"], ["ReviewTestAssignment","Others' work"],
["Others' work",'Reviews for "ReviewTestAssignment"']],
]
normal_test_paths.each do |test_path| #loop to generate a test for every entry in test_paths. This will also ensure DB is reset between tests.
description_list = []
test_path.each do |path_step|
description_list.append(path_step[0])
end
path_description = description_list.join(' -> ')
path_description.concat(" -> Request a new submission to review -> Begin")
it "user can take path #{path_description} to begin a review" do
login_as(@student3.name)
test_path.each do |path_step|
where_to_go = path_step[0]
expected_text = path_step[1]
if where_to_go.include? '/' #if user navigates to page using direct url
visit where_to_go
else #if user navigates to page by normal means
click_on(where_to_go)
end
expect(page).to have_content expected_text
end
choose "topic_id"
click_on("Request a new submission to review")
expect(page).to have_content "Begin"
click_on("Begin")
expect(page).to have_content "New Review for ReviewTestAssignment"
end
end
There is further evidence, in addition to the empirical data produced by the above test, that this bug does not exist - the lack of reproducibility. Over the course of the past two semesters, there have been roughly 130 students participating in the course. Each student is required to do at least 2 reviews for each of the 3 peer reviewed assignments. This totals up to at least 780 reviews. Given that students are able to do up to 4 reviews per assignment for extra credit, this can really be as high as 1,170 reviews. Considering there has only been one report of this bug, with no further evidence that it exists (either given then, or present at this time), it is likely that this bug report was a fabrication.
Results
When run in a local development environment, all tests within the suite pass. Further, when the pull request was put in, all tests run by Travis CI pass.
Conclusion
Given that this bug was not able to be reproduced by any of our testing, including a suite of automated tests, it is our opinion that this bug does not exist.
Testing Video
Issue #1211
Reported Problem
Issue #1211: Review is not possible during resubmission period, even when explicitly enabled. Essentially, when a review period overlaps with the second submission period, the first review period is not available (the submission period takes priority).
Investigation Approach
As seen in the issue itself, there was a previous fix for this error but it was never implemented due to it "failing tests." Upon investigating the "failed tests" documented we found that the pull request just failed to merge, and therefore the Travis CI tests counted it as a failure. We added Varun's fix back and made a pull request to see how it failed specifically, and the Travis CI result showed that it was failing in the features folder due to a NoMethodError.
After arriving at this initial conclusion, I began trying to figure out how to fix this error. My first assumption was that since I ran into no issues locally running the test bench, that it was perhaps the order it was tested in. I opened Issue #16 in our forked repo to run a bisect command, to see what the shortest path to failure was. What I found was the shortest path to failure. I then opened Issue #17, investigating that failure path. What I hadn't realized was that the seed Travis CI used did not fail the test at all locally, but rather failed two separate tests (that never fail on Travis CI). From here on out I began running a variety of rspec tests to figure out if it was just the seed, if it was an test-ordering issue, or if it was the test itself.
Eventually, I ran out of paths and after contacting my mentor and Dr. Gehringer, I decided to try and re-write the tests that failed. After inspecting the test code that was failing I realized that there was really no way it could be the tests failing due to old and faulty code. What it looked like was that the children_node_ng somehow received an empty child_node. After further investigation, it appeared to be related to Varun's change, which I then discarded and began working from scratch.
Solution
Similarly to Varun's fix, my fix was in the student_task_helper.rb file, specifically for the check_reviewable_topics method.
The line of code at fault can be seen below. As we can see, a topic can only be reviewed if it is not within a "submission" stage, which works fine when no stages overlap. However, as soon as the review stage overlaps with the submission stage, you can no longer perform a review.
return true if !assignment.topics? and assignment.get_current_stage != "submission"
The fix is fairly simple, and I was over-complicating it for the majority of this project. You can simply just check to make sure that the current stage is a review stage, rather than a submission stage. This was done by changing the != to a == and the submission to a review. Now it no longer matters if it is within a submission window or not, as long as it is within the review stage.
return true if !assignment.topics? and assignment.get_current_stage == "review"
Test Plan
Due to spending so much time investigating false paths, I didn't end up having much time to write exhaustive tests to establish this change. In theory, the way the review_assignment_spec.rb is written should already check this functionality to a degree. However, I added one more test within this spec file:
it "is able to reivew when submissions overlap with review window" do
create(:assignment_due_date, deadline_type: DeadlineType.where(name: "submission").first, due_at: DateTime.now.in_time_zone + 1.day)
create(:assignment_due_date, deadline_type: DeadlineType.where(name: "review").first, due_at: DateTime.now.in_time_zone + 4.day)
submit_to_topic
user = User.find_by(name: "student2065")
stub_current_user(user, user.role.name, user.role)
visit '/student_task/list'
visit '/sign_up_sheet/sign_up?id=1&topic_id=1'
visit '/student_task/list'
click_link "TestAssignment"
click_link "Others' work"
find(:css, "#i_dont_care").set(true)
click_button "Request a new submission to review"
expect(page).to have_content "No previous versions available"
end
The theory behind this test is that currently, all review spec tests had the same deadlines for submission and review, so I staggered them similarly to how they were staggered in the issue documentation to ensure that the fix had been met. I also tested this manually on my local system, ensuring that I could review a test if I impersonated a user.
Fix Verification
- At first, create an assignment "Test Demo Assignment" by logging into Expertiza as an Instructor. (username: instructor6, password: password).
- Navigate to Manage->Assignments.
- Create a new assignment "Test Demo Assignment" and save it to "Test_Demo" directory.
- Set the rubrics.
- Set submission deadline to be 2 days from now.
- Set review deadline to be 4 days from now.
- Enable meta-review and reviews in both phases: submission and review.
- Add student user "student2065" as a participant on this assignment.
- Log out as instructor and login as student: student2065 (username: student2065, password: password).
- Navigate to "Test Demo Assignment". Both the links, submission and review are accessible in the submission period itself.
- Click on "Your Work". The student can submit his work in submission period.
- Click on "Others' Work". The student can review other peers' work in the submission period.
Results
When run in a local development environment, all tests within the suite pass. Further, when the pull request was put in, all tests run by Travis CI pass.
Conclusion
In conclusion, from what I can see locally, this bug has been fixed. As I quickly found out dealing with Travis CI errors, its possible that this will somehow still not work correctly when pushed to the beta branch, but I can conclusively say that it should.