Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
-+-
 Reporter:  Matthijs Kooijman|Owner:  Matthijs
 |  Kooijman
 Type:  Bug  |   Status:  closed
Component:  Testing framework|  Version:  master
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"2a2ea4ee18fdcf2c95bf6435bc63b74623e3085b" 2a2ea4ee]:
 {{{
 #!CommitTicketReference repository=""
 revision="2a2ea4ee18fdcf2c95bf6435bc63b74623e3085b"
 Refs #31117 -- Made various tests properly handle unexpected databases
 aliases.

 - Used selected "databases" instead of django.db.connections.
 - Made routers in tests.migrations skip migrations on unexpected
   databases.
 - Added DiscoverRunnerGetDatabasesTests.assertSkippedDatabases() hook
   which properly asserts messages about skipped databases.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.ea5fcfe911b8e1faaebc21050c74b4ef%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
-+-
 Reporter:  Matthijs Kooijman|Owner:  Matthijs
 |  Kooijman
 Type:  Bug  |   Status:  assigned
Component:  Testing framework|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"f34be5294d8bd9530079525fb56e661816a63e20" f34be52]:
 {{{
 #!CommitTicketReference repository=""
 revision="f34be5294d8bd9530079525fb56e661816a63e20"
 Refs #31117 -- Moved get_connection_copy() test hook to a module level.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.b2311a603ddac49a90551c26171e98ee%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
-+-
 Reporter:  Matthijs Kooijman|Owner:  Matthijs
 |  Kooijman
 Type:  Bug  |   Status:  closed
Component:  Testing framework|  Version:  master
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Mariusz Felisiak ):

 * status:  assigned => closed
 * resolution:   => fixed


Comment:

 In [changeset:"b64b1b2e1a17da3fd702cde4bfc2b6e0689b4348" b64b1b2e]:
 {{{
 #!CommitTicketReference repository=""
 revision="b64b1b2e1a17da3fd702cde4bfc2b6e0689b4348"
 Fixed #31117 -- Isolated backends.base.test_creation.TestDbCreationTests.

 Previously, this test could modify global state by changing
 connection.settings_dict. This dict is a reference to the same dict as
 django.db.connections.databases['default'], which is thus also changed.
 The cleanup of this test would replace connection.settings_dic` with a
 saved copy, which would leave the dict itself modified.

 Additionally, create_test_db() would also modify these same dicts, as
 well as settings.databases['default']['NAME'] by adding a "test_"
 prefix, which is what can cause problems later.

 This patch:
  - makes a complete copy of the connection and work on that, to improve
isolation.
  - calls destroy_test_db() to let that code clean up anything done by
create_test_db().
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.b5c20d4f9184ebf12987522718ce3a96%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
-+-
 Reporter:  Matthijs Kooijman|Owner:  Matthijs
 |  Kooijman
 Type:  Bug  |   Status:  assigned
Component:  Testing framework|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Matthijs Kooijman):

 * needs_better_patch:  1 => 0


Comment:

 I completely rewrote the patch, as suggested by Felix, and it should be
 ready for review now.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.2b4a39859904e4c32c7fbee4c3a8c29b%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
-+-
 Reporter:  Matthijs Kooijman|Owner:  Matthijs
 |  Kooijman
 Type:  Bug  |   Status:  assigned
Component:  Testing framework|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by GitHub ):

 In [changeset:"c159bacebaa256a7d171d00a3eb14c4ef8357f7c" c159bace]:
 {{{
 #!CommitTicketReference repository=""
 revision="c159bacebaa256a7d171d00a3eb14c4ef8357f7c"
 Refs #31117 -- Isolated
 backends.sqlite.test_creation.TestDbSignatureTests.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.11cb5cc6207f57f8ee9fb65a0bbc5fc7%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
-+-
 Reporter:  Matthijs Kooijman|Owner:  Matthijs
 |  Kooijman
 Type:  Bug  |   Status:  assigned
Component:  Testing framework|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by felixxm):

 * status:  new => assigned
 * needs_better_patch:  0 => 1
 * has_patch:  0 => 1
 * owner:  nobody => Matthijs Kooijman


Comment:

 [https://github.com/django/django/pull/12247 PR]

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.5c0dd12b9f51fa070dc54fe2cd304edc%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
---+
 Reporter:  Matthijs Kooijman  |Owner:  nobody
 Type:  Bug|   Status:  new
Component:  Testing framework  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  0  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+

Comment (by Matthijs Kooijman):

 I highly suspect that the reason Jenkins does not show this problem, is
 because it's database config specifies `['TEST']['NAME']` rather than just
 `['NAME']`. While the former has `test_` prefixed, the latter is used as-
 is, so re-running the database does not actually change the database name,
 so this problem does not surface. I have not been able to verify this
 completely yet (since the PR I was using for testing was closed), but I'm
 pretty confident this was the case.

 In any case, the PR can now again run the testsuite completely
 succesfully. Only things open are:
  - How to prevent concurrency issues accessing this new database when
 multiple testcases use it? Normally, this is handled by making the
 testrunner make clones of the database, but I can't see how that would
 work here. Any suggestions?
  - What name to use for this new database? 'unused' does not seem too
 great.
  - The Jenkins database configuration needs to be changed to configure
 this new database. Is there anything I can do for that?
  - https://github.com/django/django/pull/12201 should be merged first.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.4694c0333517048918368fb2635c8f9f%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
---+
 Reporter:  Matthijs Kooijman  |Owner:  nobody
 Type:  Bug|   Status:  new
Component:  Testing framework  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  0  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+

Comment (by Matthijs Kooijman):

 Turns out the problem with restoration of settings also exists in
 `backends.sqlite.test_creation`. This was not previously a problem because
 `backends.base.test_creation.TestDbCreationTests` would sever the
 reference between `connection.settings_dict` and
 `connections.databases['default']`, but with that fixed, this problem is
 not exposed in the sqlite tests. I've added a fix for this in the PR as
 well.

No, but it doesn't contain anything unusual. Jenkins runs the entire
 test suite without a parallel flag, that's why it works. For example

 This does not seem true after all. Further investigation (using some dummy
 commits to trigger Jenkins builds with extra debug output) shows that
 Jenkins puts `DJANGO_TEST_PROCESSES=1` in the environment, which limits
 the build to a single process, so that cannot be the culprit here. I've
 been doing some experiments in https://github.com/django/django/pull/12248
 to figure out why Jenkins does not have this problem, but I'm having some
 problems getting the right debug output from Jenkins. I'll update here
 when I figure out something definitive.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.279926f29e2b5784cb745ae3f1b2b541%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
---+
 Reporter:  Matthijs Kooijman  |Owner:  nobody
 Type:  Bug|   Status:  new
Component:  Testing framework  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  0  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+

Comment (by Matthijs Kooijman):

 Oh, never mind the last part of my previous comment. Maybe I failed to
 stress that this only fails with MySQL and Postgres, while SQLite is
 unaffected (because it uses no NAME for the in-memory database used by
 default). So running with mysql and `--parallel 1` does indeed fail:

 {{{
 ./runtests.py multiple_database.tests backends.base backends.tests
 --parallel 1
 }}}

 But removing `-parallel makes things work again, so there is likely some
 ordering difference there.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.869e761242b603fc8cea1c6eb4370aad%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
---+
 Reporter:  Matthijs Kooijman  |Owner:  nobody
 Type:  Bug|   Status:  new
Component:  Testing framework  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  0  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+

Comment (by Matthijs Kooijman):

 `./runtests.py multiple_database.tests backends.base backends.tests`
   works without any issues.

 Oh, interesting. Parallelization might indeed be related (I can imagine
 that when parallelization causes the two problematic tests to be ran in
 different threads and/or different order, this might not break).

 However, something else seems to be going on (as well maybe). The command
 you gave indeed works, but when I add `--parallel 1` (which would ensure
 the problematic tests are ran in the problematic order), it still works:

 {{{
 ./runtests.py multiple_database.tests backends.base backends.tests
 --parallel 1
 }}}

 Maybe there is some tests that, when ran in between, prevents this problem
 from occuring? OTOH, when I run the *entire* test suite, with
 parallelization, IIRC the problem *also* occurs.

 Regardless, I believe my analysis and proposed solution still hold. I just
 tried the fix and it seems to work, so pullrequest coming up.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.a7cf9209432d032e38805485e1ea39bf%40djangoproject.com.

Re: [Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests. (was: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests)

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests.
---+
 Reporter:  Matthijs Kooijman  |Owner:  nobody
 Type:  Bug|   Status:  new
Component:  Testing framework  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  0  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+
Changes (by felixxm):

 * stage:  Unreviewed => Accepted
 * component:  Uncategorized => Testing framework


Comment:

 > Is the configuration for the Jenkins tests published somewhere? Maybe
 that would offer a clue about this difference.

 No, but it doesn't contain anything unusual. Jenkins runs the entire test
 suite without a `parallel` flag, that's why it works. For example
 {{{
 > ./runtests.py multiple_database.tests backends.base backends.tests
 }}}
 works without any issues.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/074.4d2b1fee423a39f56b9aca291214c374%40djangoproject.com.

[Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests

[Django]

#31117: ThreadTests fails due to double test_ prefix caused by 
TestDbCreationTests
-+
   Reporter:  Matthijs Kooijman  |  Owner:  nobody
   Type:  Bug| Status:  new
  Component:  Uncategorized  |Version:  master
   Severity:  Normal |   Keywords:
   Triage Stage:  Unreviewed |  Has patch:  0
Needs documentation:  0  |Needs tests:  0
Patch needs improvement:  0  |  Easy pickings:  0
  UI/UX:  0  |
-+
 While working on testcases for #26552 and #31051 I ran into some problems
 with
 testing against a local MySQL and Postgres server. I initially thought my
 testcases broke things, but it turns out I can see the same broken
 behaviour on
 a clean master. Hence this ticket.

 In short, what happens is that
 `backends.base.test_creation.TestDbCreationTests` runs `create_test_db` to
 verify migration is (not) run depending on the `MIGRATE` settings.
 However,
 this has two problems:

  - This runs `create_test_db` on an already initialized database, leading
 to
double initialization (in particular, it adds a *second* `test_` prefix
 to the
database name, which produces an unexpected database name.
  - `create_test_db` has side effects that are not properly cleaned up.
 Some of them
are reverted by restoring `settings_dict` (see below), but not all of
 them
(more on this below).

 === Possible solution

 A solution to both issues could be to use a separate database, that is not
 normally used by other tests and thus not initialized by the test runner.
 This
 test can then freely call `create_test_db` to initialize it (possible even
 actually creating the database), and then call `destroy_test_db` to clean
 up
 everthing again.

 I have alredy been working on adding an extra database like this, since
 this
 was also a possible solution to problems I had with
 https://github.com/django/django/pull/12166

 === To reproduce

 On my system, the partial reverting leads to an exception in a later test
 due
 to a fairly obscure interaction between various parts (details below).

 To reproduce this exception, I've used the commands below. Note that the
 `multiple_databases.tests` are not related to this bug directly, but used
 to
 work around #31055.

 {{{
 $ ./runtests.py --settings test_postgresql multiple_database.tests
 backends.base.test_creation.TestDbCreationTests backends.tests.ThreadTests
 --parallel 1
   File "/home/matthijs/docs/src/upstream/django/venv/lib/python3.7/site-
 packages/psycopg2/__init__.py", line 126, in connect
 conn = _connect(dsn, connection_factory=connection_factory, **kwasync)
 psycopg2.OperationalError: FATAL:  database "test_test_django_main" does
 not exist

 $ ./runtests.py --settings test_mysql multiple_database.tests
 backends.base.test_creation.TestDbCreationTests backends.tests.ThreadTests
 --parallel 1

   File "/home/matthijs/docs/src/upstream/django/venv/lib/python3.7/site-
 packages/MySQLdb/connections.py", line 179, in __init__
 super(Connection, self).__init__(*args, **kwargs2)
 MySQLdb._exceptions.OperationalError: (1044, "Access denied for user
 'test_django'@'localhost' to database 'test_test_django_main'")
 }}}

 Interestingly enough, this happens for me locally, but not with the
 automatic
 testing on Jenkins. I originally suspected that this was because my MySQL
 permissions were set up strictly (only allowing access to
 `test_django_*`), but
 for postgresql, the error was not about permissions. Also, when I relaxed
 the
 MySQL permissions, the error only changed to `(1049, "Unknown database
 'test_test_django_main'")`.

 Is the configuration for the Jenkins tests published somewhere? Maybe that
 would offer a clue about this difference.

 === Analysis

 I dug a little deeper to figure out why this exception occurs exactly.
 This is what happens:

 First, `create_test_db` prepends `test_` to the database name, and
 configures this in two places:

 {{{
 settings.DATABASES[self.connection.alias]["NAME"] = test_database_name
 self.connection.settings_dict["NAME"] = test_database_name
 }}}

 See
 
https://github.com/django/django/blob/cebd41e41603c3ca77c5b29d6cd20c1bff43827f/django/db/backends/base/creation.py#L33

 Since the test runner has previously called `create_test_db`, the database
 name now has a `test_test_` prefix.

 At this time, self.connection.settings_dict is the same dict as
 `django.db.connections.databases`, so that one is also changed.

 At the end of the test, `connection.settings_dict` is restored to a copy
 made before the test. This replaces the entire dict instead of modifying
 the
 dict, so `django.db.connections.databases` is *not* restored and still has
 the `test_test_` prefix.

 {{{