1325838 - Stop using Python asserts for conditions that should be checked in production

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P3)

Description

[Ed Morley [:emorley]]

`assert` statements are removed depending on the Python optimisation level, so should not be used outside of checks that are intended to be test or debug only.

However there are several places where we currently use them, when we should use if statements and explicit raises instead.

eg:

https://github.com/mozilla/treeherder/search?utf8=%E2%9C%93&q=assert+-path%3Atests%2F&type=Code

Comment 1

[Arjun Nair]

Hey,

Is this just a simple case of converting all the asserts to if statements and raising the required exceptions?
I'd like to take a look.

Comment 2

[Ed Morley [:emorley]]

Yeah that sounds right :-)

Comment 3

[Arjun Nair]

Alright, I'll try and get this done. Do you only need this changed in the places you mentioned in your initial comment?

Some input will be appreciated. (what type of errors to raise etc)

Comment 4

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3128

Comment 5

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3348

Comment 6

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/8a1fb7befbd30310fb5808bbaf45715ccafd0052
Bug 1325838 - Remove non-testing assert statements (#3348)

* Replace settings asserts with ImproperlyConfigured
* Replace attribute asserts with AttributeError
* Replace duplicate key assert with a custom exception
   There doesn't seem like a better exception to use here so I added a
   custom one.
* Check the id exists
* Replace asserts with manual AssertionErrors

Comment 7

[Cameron Dawson [:camd]]

Comment on attachment 8959565 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3348

Thanks!!

Comment 8

[Cameron Dawson [:camd]]

I noticed a couple other ``assert`` statements in one of our model managers.  I'm not clear if those should be converted as well?

https://github.com/mozilla/treeherder/blob/5052c5dfaae007011cfb8ec7e025a89b8974263c/treeherder/model/models.py#L1159-L1164

Ed: What do you think?

Comment 9

[Ed Morley [:emorley]]

Yeah let's change those too - something like this from the obsoleted previous PR should work (or whatever else looks better):
https://github.com/mozilla/treeherder/pull/3128/files#diff-e432623caa985e5ac4d48814de254a58R1164

Comment 10

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3354

Comment 11

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/e6aec72dd9b614c46141930df7c8a797328411de
Bug 1325838 - remove asserts in MatcherManager classmethods (#3354)