Fix finish_job bug, add tests #328
Conversation
Method was unused outside the class other than in a test where it was mocked out
EE2status attempting to access get_catalog_utils().catalog caused finish_job() to throw an exception and consequently the job runner to freak out: kbase/JobRunner#43 Accessed the catalog directly and added a unit test that covers the code in question.
| job_id=job_id, requested_job_perm=JobPermissions.WRITE, as_admin=False | ||
| ), | ||
| call( | ||
| job_id=job_id, requested_job_perm=JobPermissions.WRITE, as_admin=False |
There was a problem hiding this comment.
Thanks black this is so much better
| "job_id": job_id, | ||
| } | ||
| ) | ||
| mongo.update_job_resources.assert_called_once_with(job_id, resources) |
There was a problem hiding this comment.
What about some negative tests? Or tests where failures are expected with bad inputs?
There was a problem hiding this comment.
The point of this PR was not to add tests, per se - it was to fix the bug re accessing the catalog client by an instance variable that doesn't exist, and write tests that cover that bug so that it can't reoccur. The happy path test was enough to cover the bug, so failure tests weren't needed.
There was a problem hiding this comment.
Testing the rest of that function would catch other issues before they happen and prevent other bugs from appearing.
There was a problem hiding this comment.
Sure, but that's true for lots of places in the code. The point of this PR is to fix and test a specific bug, not to add tests in general.
There was a problem hiding this comment.
The point is to fix a bug and, hopefully, prevent others from taking its place due to poorly tested code. Please write some more tests to capture other failure conditions.
There was a problem hiding this comment.
and, hopefully, prevent others from taking its place due to poorly tested code
Hmm, looks like we're at an impasse, because AFAIC, that is not part of the point of this PR. I'll bug @sychan to be a tiebreaker here.
There was a problem hiding this comment.
Works for me. The past few times you complained of errors happening from untested code and from things that reviewers didn't catch, I figured this would be an opportunity to avoid that. Letting @sychan decide whether more work is warranted here is fine.
There was a problem hiding this comment.
The past few times you complained of errors happening from untested code and from things that reviewers [note: and I, and 5 linters -g] didn't catch
That's still a valid complaint, and I'll continue to address it by adding tests for code that I touch. It's just that
- if I add unit tests for every piece of code I see that doesn't have unit tests I'll never work on the actual feature I'm supposed to be working on, and
- I want to keep this PR focused on the bugfix.
It became clear to me early on that unless I restrict adding tests to code I actually make changes to I won't finish, as demonstrated by the fact that I'm already several weeks into this process and just barely started on the feature.
There was a problem hiding this comment.
Bill is correct on the need for testing failed runs, and given how important EE2 to the core functioning of our platform - we do, desperately, need more pervasive testing on the EE2 codebase. And it is ironic that Gavin is the one arguing for not adding tests, the shoe isn't so comfortable when its on the other foot!
The mitigating circumstance is that we are weeks (if not coming up months behind) on getting basic features implemented and we're 3 weeks from the end of the quarter, after which we are likely to take a huge hit on resourcing for the Bulk Import work, and I would actually like Gavin to go somewhat wide on the EE2 codebase, instead of deep at this point. I'm going to say we let this slide, and have Gavin move on - with the understanding that Gavin will work on incrementally improving the code coverage in EE2 as he does future PRs.
While it's true I often ask for more tests in PR reviews, I don't think I've ever asked for tests for code that was not added or modified by the PR. |
Description of PR purpose/changes
EE2status attempting to access get_catalog_utils().catalog caused
finish_job() to throw an exception and consequently the job runner to freak out:
kbase/JobRunner#43
Accessed the catalog directly and added a unit test that covers the code
in question.
Also reduced the EE2Status API by making a method that was only used in tests "private".
Jira Ticket / Github Issue
Testing Instructions
Dev Checklist:
Updating Version and Release Notes (if applicable)