Skip to content
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

Fixed TransactionalTests.test_password_with_at_sign() isolation on Oracle. #17148

Merged
merged 1 commit into from Aug 4, 2023

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Aug 4, 2023

Without this patch:

python runtests.py backends.oracle.tests.TransactionalTests.test_password_with_at_sign
Testing against Django installed in '/django/django' with up to 8 processes
Found 1 test(s).
Creating test database for alias 'default'...
Creating test user...
System check identified no issues (0 silenced).
F
======================================================================
FAIL: test_password_with_at_sign (backends.oracle.tests.TransactionalTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/django/tests/backends/oracle/tests.py", line 113, in test_password_with_at_sign
    with self.assertRaises(DatabaseError) as context:
AssertionError: DatabaseError not raised

----------------------------------------------------------------------

----------------------------------------------------------------------
Ran 1 test in 0.544s

FAILED (failures=1)
Destroying test database for alias 'default'...
Destroying test user...
Destroying test database tables...

With the patch:

python runtests.py backends.oracle.tests.TransactionalTests.test_password_with_at_sign
Testing against Django installed in '/django/django' with up to 8 processes
Found 1 test(s).
Creating test database for alias 'default'...
Creating test user...
System check identified no issues (0 silenced).
.
----------------------------------------------------------------------
Ran 1 test in 2.132s

OK
Destroying test database for alias 'default'...
Destroying test user...
Destroying test database tables...

@felixxm felixxm requested a review from a team August 4, 2023 11:11
@felixxm
Copy link
Member Author

felixxm commented Aug 4, 2023

buildbot, test on oracle.

@felixxm felixxm force-pushed the isolation-test_password_with_at_sign branch from 27e2df4 to 7b7d4d4 Compare August 4, 2023 11:13
@@ -103,15 +103,17 @@ def test_hidden_no_data_found_exception(self):
cursor.execute('DROP TRIGGER "TRG_NO_DATA_FOUND"')

def test_password_with_at_sign(self):
from django.db.backends.oracle.base import Database

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question, is this new local import part of the solution? From the code it looks like oracle's DatabaseError is not an child of Django's DatabaseError?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my question (to better understand the fix), LGTM 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to catch a different error, because we're calling a different method that doesn't wrap database-specific errors with Django's DatabaseError.

@felixxm
Copy link
Member Author

felixxm commented Aug 4, 2023

buildbot, test on oracle.

@felixxm
Copy link
Member Author

felixxm commented Aug 4, 2023

Thanks both for checking 👍

@felixxm felixxm merged commit 0336aa6 into django:main Aug 4, 2023
28 checks passed
@felixxm felixxm deleted the isolation-test_password_with_at_sign branch August 4, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants