Code review comment for lp:~salgado/offspring/private-projects

On Wed, 2011-11-30 at 14:57 +0000, Kevin McDermott wrote:
> Review: Approve
>
> #1
>
> +BUILDRESULTS_DIRECTORY = '/srv/offspring.com/www/builds/'
>
> Is this the right path?

This is part of path-independence; I've already removed it from this
branch but it looks like you merged an old revision when you reviewed.

>
> #2
>
> Where is the migration used?

It's not; Offspring doesn't use south (Cody had concerns with it) so the
migration scripts have to be applied manually.

>
> #3
>
> + def test_get_projects_filters_private_objects(self):
> No docstring.

Done

> #4
>
> +class ReleaseTests(
> + TestCase,
> ReleaseOrLexbuilderOrBuildResultOrBuildRequestTestsMixin):
> + model = Release
> +
> + def factoryMethod(self, project):
> + return factory.make_release(
> + build=factory.make_build_result(project=project))
> +
> +
> +class LexbuilderTests(
> + TestCase,
> ReleaseOrLexbuilderOrBuildResultOrBuildRequestTestsMixin):
> + model = Lexbuilder
> +
> + def factoryMethod(self, project):
> + return factory.make_lexbuilder(
> + current_job=factory.make_build_result(project=project))
> +
> +
> +class BuildResultTests(
> + TestCase,
> ReleaseOrLexbuilderOrBuildResultOrBuildRequestTestsMixin):
> + model = BuildResult
> + factoryMethod = factory.make_build_result
> +
> +
> +class BuildRequestTests(
> + TestCase,
> ReleaseOrLexbuilderOrBuildResultOrBuildRequestTestsMixin):
> + model = BuildRequest
> + factoryMethod = factory.make_build_request
>
> Couldn't this be done as a parent class, something like
> BasePrivateModelTestCase or something?

I'm not sure I understand. What would we put in the parent class? Both
the model and the factory method are different on every class and they
already share a parent class which has the actual tests.

>
> #5
>
> +class ProjectTests(TestCase):
>
> There are no docstrings explaining any of the tests...

I'm not convinced using docstrings in test methods is a good idea as
nobody is going to be using these tests methods (i.e. they're not an
API), but when I have a test which is not trivial I write some comments
around the interesting lines. I do that for a couple tests here, but I
don't think the others need any comments as they'd just be the test's
name with s/_/ /

> #6
>
> +from .test_models import *
>
> Any idea why we actually need this? (I know we do, but is there a way
> we can drop it?) My concern is that missing tests from there won't
> necessarily affect localised test runs, but could be missed from just
> running test-web.

Django requires it.

> Also...see PEP8
>
> - Relative imports for intra-package imports are highly
> discouraged.
> Always use the absolute package path for all imports.
> Even now that PEP 328 [7] is fully implemented in Python 2.5,
> its style of explicit relative imports is actively discouraged;
> absolute imports are more portable and usually more readable.

Ok, changed.

>
> #7
>
> === added directory 'lib/offspring/web/queuemanager/sql'
> === added file
> 'lib/offspring/web/queuemanager/sql/project.postgresql_psycopg2.sql'
>
> What are these used for? Is there a speed benefit from using sqlite
> in tests?

Yes, it's incredibly faster to run using sqlite3, but there's also a
test-web-postgres target to run the test-suite using postgres.

> Personally, I'd prefer to run the tests against the database we use in
> production i.e. postgresql and not have SQL files in separate
> directories for this purpose...

We can make test-web use postgres and have a test-web-sqlite3.

>
> #8
>
> +class Release(models.Model, MaybePrivateMixin):
> +class BuildResult(models.Model, MaybePrivateMixin):
> +class Lexbuilder(models.Model, MaybePrivateMixin):
>
>
> + # Using PublicOnlyObjectsManager here we ensure that any
> oversights when
> + # updating existing uses of Release.objects won't leak anything
> related to
> + # private projects.
>
> How about defining a base class for these MaybePrivateMixin objects
> rather than redeclaring he same thing and comments each time, and just
> subclass for the various models that use it?

That's tricky because the Managers have to be declared on a models.Model
instance and if I make MaybePrivateMixin a subclass of that, Project
will end up inheriting from it twice: via MaybePrivateMixin and
AccessGroupMixin, and that seems to case some weird issues. I'm sure I
could create two separate mixins to workaround this but then I'm not
sure it's worth it anymore.

> #9
>
> + hostname = os.popen('hostname').read().strip()
> + config.set('DEFAULT', 'hostname', hostname)
> + config.set('DEFAULT', 'username', os.environ['USERNAME'])
>
> I reimplemented this in add-path-independence, but note that you can use os.uname to get the hostname...

This is also not present here anymore.

> #10
>
> +base_dir: /srv
>
> -development_pool: %(base_dir)s/builds/
> -production_pool: %(base_dir)s/partners/
> +development_pool: /srv/builds/
> +production_pool: /srv/partners/
>
> It's not clear why this is being changed? Is this right, and, if it is, wouldn't /srv/offspring, /srv/offspring/builds and /srv/offspring/partners be more explicit?

Ditto.

> And
> In requirements.web.txt
>
> psycopg2>=2.0.6, <=2.4.1
>
> Otherwise we'll run into the 2.4.2 transaction-not-being-fixed-in-django-1.3 bug.

That was done in another branch, so I'd rather not do it here as it's
not related to those changes I'm doing and would cause yet another
conflict later on when I merge this back into the linaro branch.