-
-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exception handler tests #3547
Exception handler tests #3547
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test looks good, but the imports are (re)ordered strangely.
Is there a reason for the non-standard import sorting? If not, just use the style I recommended, please.
from http.client import CannotSendRequest | ||
import pytest | ||
|
||
from django.core.exceptions import FieldDoesNotExist | ||
from psycopg.errors import BadCopyFileFormat | ||
import pytest | ||
from django.core.exceptions import FieldDoesNotExist | ||
from mathesar.utils.connections import BadInstallationTarget | ||
from db.functions.exceptions import UnknownDBFunctionID | ||
from sqlalchemy.exc import IntegrityError | ||
from http.client import CannotSendRequest | ||
|
||
from db.functions.exceptions import UnknownDBFunctionID | ||
from mathesar.utils.connections import BadInstallationTarget | ||
import mathesar.rpc.exceptions.error_codes as err_codes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an automated tool reorder these imports? The new order is pretty non-standard. I'd rather have:
from http.client import CannotSendRequest
from django.core.exceptions import FieldDoesNotExist
from psycopg.errors import BadCopyFileFormat
import pytest
from sqlalchemy.exc import IntegrityError
from db.functions.exceptions import UnknownDBFunctionID
import mathesar.rpc.exceptions.error_codes as err_codes
from mathesar.utils.connections import BadInstallationTarget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The imports should be grouped into:
- standard library
- 3rd party libraries
- local libraries (our code)
Sorting should be lexicographical by non-keyword within those groups.
import pytest | ||
|
||
from modernrpc.exceptions import RPCException | ||
from psycopg.errors import BadCopyFileFormat | ||
from django.core.exceptions import FieldDoesNotExist | ||
from mathesar.utils.connections import BadInstallationTarget | ||
from db.functions.exceptions import UnknownDBFunctionID | ||
from http.client import CannotSendRequest | ||
|
||
from mathesar.rpc.exceptions.handlers import handle_rpc_exceptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are also sorted strangely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing with @Anish9901 , I'm convinced there is a logic to the sorting of the imports in these test files.
@Anish9901 , Please add a comment or two about that logic, and then go ahead and merge this.
I've sorted & grouped the imports based on "what's being tested" rather than "where are we importing from". Furthermore, if we try to see the imports through the lens of Arrange, Act and Assert we'll notice that the following imports are required for the Arrange step for the tests defined: from modernrpc.exceptions import RPCException
from psycopg.errors import BadCopyFileFormat
from django.core.exceptions import FieldDoesNotExist
from mathesar.utils.connections import BadInstallationTarget
from db.functions.exceptions import UnknownDBFunctionID
from http.client import CannotSendRequest Similarly, we can think of the following import as something which will be Acted upon or rather the function that is to be tested. from mathesar.rpc.exceptions.handlers import handle_rpc_exceptions The first line is reserved for the imports that would be imported multiple times throughout our test files and for any imports that are required for rudementry python/pytest functions e.g. |
Related to #3524
This adds tests for the error handling decorator
handle_rpc_exceptions
, and verifies a function wrapped always raises aRPCException
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin