add sample validation page for wetlab #3213
Conversation
|
Thank you @Sarayu. I asked @charles-cowart for a review but could you add tests to your new code? |
| @@ -45,5 +45,19 @@ def test_get_missing_argument(self): | |||
| response.body.decode('ascii')) | |||
|
|
|||
|
|
|||
| class TestSampleValidation(BaseAdminTests): | |||
| def test_get(self): | |||
There was a problem hiding this comment.
As mentioned during the meeting, a good unittest should canvas the common things that might break your method/function. For example, calling post without one or both of the post_args, or giving a value for qid or snames that is obviously bad. This confirms that error-handling exists and is working properly.
There was a problem hiding this comment.
Modified unit test to also check error handling of post with invalid qid
| 'snames': 'SKB1.640202 SKB2.640194 BLANK.1A BLANK.1B' | ||
| } | ||
| response = self.post('/admin/sample_validation/', post_args) | ||
| self.assertEqual(response.code, 200) |
There was a problem hiding this comment.
Some coders will return 200 as long as the command executed without failure on the server-side. It's good to check the response.text and confirm it does not contain error messages and contains at least some string of text you're expecting.
There was a problem hiding this comment.
Modified unit test to check that the response body contains the inputted sample names
| body = response.body.decode('ascii') | ||
| for name in snames: | ||
| self.assertIn(name, body) | ||
|
|
There was a problem hiding this comment.
@sarayupai Good job! But note that the test is called 'test_post' and you're testing positive and negative outcomes in this test. It's not obvious to the reader why this test should return 500. You have to compare each input to realize the only difference is 2 and perhaps that's an invalid id. It would be good to add a comment here summarizing what you're doing and what you expect to see happen.
There was a problem hiding this comment.
Added comments indicating the positive and negative tests
|
@charles-cowart, is this ready to be merged? |
|
Thank you @sarayupai and @charles-cowart ! |
No description provided.