Phase 4 error handling#360
Open
mewilker wants to merge 2 commits intosoftwareconstruction240:mainfrom
Open
Conversation
Contributor
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
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.
Explanation
The Phase 4 tests introduce a test to students that makes sure that when a
SQLExceptionor more appropriately aDataAcessExceptionis thrown due to the inability to get a connection, the error is handled as an "Internal Server Error". While this test is good and teaches students to handle unforeseen errors appropriately, it references the Phase 3 spec. While it would be nice if this was implemented and Phase 3 and carried over seamlessly in Phase 4, my experience is that most students don't begin to error codes until this Phase 4.If their code works properly in Phase 3, they never really run into the 500 response. Sure, it would be nice for debugging if they caught their null pointer exceptions and sent a 500 back, but most of them don't do that. They typically either A) let Javalin execute its default behavior (catch the exception and print it to the console) or B) have an else statement for one of their status codes (like the 400 for bad request) and have that returned. They may struggle with this through Phase 3, but ultimately, they pass the
StandardAPITestswithout ever having to worry about the 500 at the bottom of each of our endpoint tables.Then comes along Phase 4. A lot of students seem confused about what the
Database Error Handling Testmeans and spend some time trying to figure out what the test is asking for. Some of them spend some time trying to figure out what they are serializing and what exactly the test wants. Now I believe @Maillman's PR to change the tests should help clarify this (found here: softwareconstruction240/chess#49) but I think there also needs to be a reference to the Phase 3 spec in the Phase 4 one. This is so students can understand where this expectation comes from. Rather than determine our expectations from the test, they are reminded in the spec how to handle database errors.Changes
This PR adds a small new section to the spec. The paragraph explains the response that is supposed to happen when connections occur. It also links to the Phase 3 spec, so they can be reminded of the format we expect.
When to Merge:
This PR could be merged as soon as the professors deem it appropriate, however since Phase 4 has already passed, it may be a good idea to wait for Winter 2026 to end.