launchpad-reviewers team mailing list archive

[Ines Almeida <mp+447883@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:flush-db-on-db-user-switch into launchpad:master.

Commit message:
Flush database on db user switch
    
This prevents the need for flushing the DB in certain tests, and will makes it more likely that we will catch db permission errors in tests


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/447883

I tested this in a test in TestGitRefScanJob (i.e., removed the DB permission needed for the test to pass, and ensured that the test failed without the `flush` in-test, and passed without the new overall db flush).

The main goal is that we might catch db permission errors sooner in tests, and removes the need to use `.flush()` in specific places in tests.

I don't think there is any need to remove other `flush()`s from tests in one MP because testing that would take a while, but it can be something we do as we go when we are updating a test
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:flush-db-on-db-user-switch into launchpad:master.

diff --git a/lib/lp/code/model/tests/test_gitjob.py b/lib/lp/code/model/tests/test_gitjob.py
index 221e7e5..3ca4fe2 100644
--- a/lib/lp/code/model/tests/test_gitjob.py
+++ b/lib/lp/code/model/tests/test_gitjob.py
@@ -40,7 +40,6 @@ from lp.code.model.gitjob import (
 from lp.code.tests.helpers import GitHostingFixture
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
-from lp.services.database.interfaces import IStore
 from lp.services.job.runner import JobRunner
 from lp.services.utils import seconds_since_epoch
 from lp.services.webapp import canonical_url
@@ -474,7 +473,6 @@ class TestGitRefScanJob(TestCaseWithFactory):
                 das,
                 [[("test", 0)]],
             )
-            IStore(repository).flush()
 
             # Create duplicated build
             self.assertRaises(
@@ -486,7 +484,6 @@ class TestGitRefScanJob(TestCaseWithFactory):
                 [[("test", 0)]],
                 git_refs=["refs/heads/test"],
             )
-            IStore(repository).flush()
 
         self.assertEqual(["refs/heads/test"], build.git_refs)
 
diff --git a/lib/lp/testing/dbuser.py b/lib/lp/testing/dbuser.py
index 3441593..eeb2bdc 100644
--- a/lib/lp/testing/dbuser.py
+++ b/lib/lp/testing/dbuser.py
@@ -17,6 +17,7 @@ from storm.zope.interfaces import IZStorm
 from zope.component import getUtility
 
 from lp.services.config import dbconfig
+from lp.services.database.sqlbase import flush_database_updates
 
 
 def update_store_connections():
@@ -56,6 +57,7 @@ def switch_dbuser(new_name):
 
     If new_name is None, the default will be restored.
     """
+    flush_database_updates()
     transaction.commit()
     dbconfig.override(dbuser=new_name)
     update_store_connections()