launchpad-reviewers team mailing list archive

[Colin Watson <mp+428087@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:factory-proxy-oci into launchpad:master.

Commit message:
Return proxied objects from various OCI factory methods

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/428087

`LaunchpadObjectFactory` issues `UnproxiedFactoryMethodWarning` when its methods return objects not wrapped in a security proxy, since that tends to result in tests that are less accurate simulations of production.

Some tests were passing an `OCIRecipeArch` to `OCIRecipe.requestBuild` instead of a `DistroArchSeries`, which previously happened to work by luck; returning proxied objects exposed this error.  I removed `makeOCIRecipeArch` entirely, since it was only used by these incorrect tests.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:factory-proxy-oci into launchpad:master.

diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index 3050f61..f214eac 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -1888,12 +1888,13 @@ class TestOCIRecipeView(BaseTestOCIRecipeView):
             paths=["refs/heads/v1.0-20.04"],
             information_type=InformationType.PRIVATESECURITY,
         )
-        recipe = self.makeOCIRecipe(
-            oci_project=oci_project,
-            git_ref=ref,
-            build_file="Dockerfile",
-            information_type=InformationType.PRIVATESECURITY,
-        )
+        with person_logged_in(self.person):
+            recipe = self.makeOCIRecipe(
+                oci_project=oci_project,
+                git_ref=ref,
+                build_file="Dockerfile",
+                information_type=InformationType.PRIVATESECURITY,
+            )
         with admin_logged_in():
             build_path = recipe.build_path
             self.makeBuild(
diff --git a/lib/lp/oci/tests/test_ocirecipe.py b/lib/lp/oci/tests/test_ocirecipe.py
index 0262c88..bc212a2 100644
--- a/lib/lp/oci/tests/test_ocirecipe.py
+++ b/lib/lp/oci/tests/test_ocirecipe.py
@@ -256,20 +256,20 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
 
     def test_requestBuild(self):
         ocirecipe = self.factory.makeOCIRecipe()
-        oci_arch = self.factory.makeOCIRecipeArch(recipe=ocirecipe)
-        build = ocirecipe.requestBuild(ocirecipe.owner, oci_arch)
+        das = self.factory.makeDistroArchSeries()
+        build = ocirecipe.requestBuild(ocirecipe.owner, das)
         self.assertEqual(build.status, BuildStatus.NEEDSBUILD)
 
     def test_requestBuild_already_exists(self):
         ocirecipe = self.factory.makeOCIRecipe()
-        oci_arch = self.factory.makeOCIRecipeArch(recipe=ocirecipe)
-        ocirecipe.requestBuild(ocirecipe.owner, oci_arch)
+        das = self.factory.makeDistroArchSeries()
+        ocirecipe.requestBuild(ocirecipe.owner, das)
 
         self.assertRaises(
             OCIRecipeBuildAlreadyPending,
             ocirecipe.requestBuild,
             ocirecipe.owner,
-            oci_arch,
+            das,
         )
 
     def test_requestBuild_triggers_webhooks(self):
@@ -282,11 +282,11 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
             }
         ):
             recipe = self.factory.makeOCIRecipe()
-            oci_arch = self.factory.makeOCIRecipeArch(recipe=recipe)
+            das = self.factory.makeDistroArchSeries()
             hook = self.factory.makeWebhook(
                 target=recipe, event_types=["oci-recipe:build:0.1"]
             )
-            build = recipe.requestBuild(recipe.owner, oci_arch)
+            build = recipe.requestBuild(recipe.owner, das)
 
         expected_payload = {
             "recipe_build": Equals(
diff --git a/lib/lp/registry/tests/test_ociprojectseries.py b/lib/lp/registry/tests/test_ociprojectseries.py
index 26b63ba..45391b6 100644
--- a/lib/lp/registry/tests/test_ociprojectseries.py
+++ b/lib/lp/registry/tests/test_ociprojectseries.py
@@ -133,17 +133,11 @@ class TestOCIProjectSeriesWebservice(TestCaseWithFactory):
                 oci_project=project, registrant=self.person
             )
             url = api_url(series)
-
-        expected_url = "{project}/+series/{name}".format(
-            project=api_url(project), name=series.name
-        )
-        self.assertEqual(expected_url, url)
-
-        ws_series = self.load_from_api(url)
-
-        self.assertThat(
-            ws_series,
-            ContainsDict(
+            expected_url = "{project}/+series/{name}".format(
+                project=api_url(project), name=series.name
+            )
+            self.assertEqual(expected_url, url)
+            series_matcher = ContainsDict(
                 {
                     "date_created": Equals(series.date_created.isoformat()),
                     "name": Equals(series.name),
@@ -154,8 +148,11 @@ class TestOCIProjectSeriesWebservice(TestCaseWithFactory):
                     "status": Equals(series.status.title),
                     "summary": Equals(series.summary),
                 }
-            ),
-        )
+            )
+
+        ws_series = self.load_from_api(url)
+
+        self.assertThat(ws_series, series_matcher)
 
     def test_get_non_existent_series(self):
         with person_logged_in(self.person):
@@ -165,9 +162,9 @@ class TestOCIProjectSeriesWebservice(TestCaseWithFactory):
             series = self.factory.makeOCIProjectSeries(
                 oci_project=project, registrant=self.person
             )
+            url = "{project}/+series/{name}trash".format(
+                project=api_url(project), name=series.name
+            )
 
-        url = "{project}/+series/{name}trash".format(
-            project=api_url(project), name=series.name
-        )
         resp = self.webservice.get(url + "trash")
         self.assertEqual(404, resp.status, resp.body)
diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py
index 0081a14..1407924 100644
--- a/lib/lp/scripts/tests/test_garbo.py
+++ b/lib/lp/scripts/tests/test_garbo.py
@@ -1088,7 +1088,7 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory):
         switch_dbuser("testadmin")
         store = IMasterStore(OCIFile)
         ocifile = self.factory.makeOCIFile()
-        ocifile.date_last_used = THIRTY_DAYS_AGO
+        removeSecurityProxy(ocifile).date_last_used = THIRTY_DAYS_AGO
         self.assertEqual(1, store.find(OCIFile).count())
 
         self.runDaily()
@@ -1115,7 +1115,7 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory):
         switch_dbuser("testadmin")
         store = IMasterStore(OCIFile)
         ocifile = self.factory.makeOCIFile()
-        ocifile.date_last_used = THIRTY_DAYS_AGO
+        removeSecurityProxy(ocifile).date_last_used = THIRTY_DAYS_AGO
         self.factory.makeOCIFile()
         self.assertEqual(2, store.find(OCIFile).count())
 
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index c6a135a..68580a4 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -139,7 +139,6 @@ from lp.oci.interfaces.ocipushrule import IOCIPushRuleSet
 from lp.oci.interfaces.ocirecipe import IOCIRecipeSet
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
 from lp.oci.interfaces.ociregistrycredentials import IOCIRegistryCredentialsSet
-from lp.oci.model.ocirecipe import OCIRecipeArch
 from lp.oci.model.ocirecipebuild import OCIFile
 from lp.oci.model.ocirecipebuildjob import (
     OCIRecipeBuildJob,
@@ -6449,7 +6448,7 @@ class LaunchpadObjectFactory(ObjectFactory):
             registrant = self.makePerson()
         if oci_project is None:
             oci_project = self.makeOCIProject(**kwargs)
-        return oci_project.newSeries(name, summary, registrant)
+        return ProxyFactory(oci_project.newSeries(name, summary, registrant))
 
     def makeOCIRecipe(
         self,
@@ -6504,14 +6503,6 @@ class LaunchpadObjectFactory(ObjectFactory):
             information_type=information_type,
         )
 
-    def makeOCIRecipeArch(self, recipe=None, processor=None):
-        """Make a new OCIRecipeArch."""
-        if recipe is None:
-            recipe = self.makeOCIRecipe()
-        if processor is None:
-            processor = self.makeProcessor()
-        return OCIRecipeArch(recipe, processor)
-
     def makeOCIRecipeBuild(
         self,
         requester=None,
@@ -6587,10 +6578,12 @@ class LaunchpadObjectFactory(ObjectFactory):
             library_file = self.makeLibraryFileAlias(
                 content=content, filename=filename
             )
-        return OCIFile(
-            build=build,
-            library_file=library_file,
-            layer_file_digest=layer_file_digest,
+        return ProxyFactory(
+            OCIFile(
+                build=build,
+                library_file=library_file,
+                layer_file_digest=layer_file_digest,
+            )
         )
 
     def makeOCIRecipeBuildJob(self, build=None):
@@ -6601,7 +6594,7 @@ class LaunchpadObjectFactory(ObjectFactory):
             build, OCIRecipeBuildJobType.REGISTRY_UPLOAD, {}
         )
         store.add(job)
-        return job
+        return ProxyFactory(job)
 
     def makeOCIRegistryCredentials(
         self, registrant=None, owner=None, url=None, credentials=None