Обсуждение: PGweb: Patches and tests

Hello everyone!

I've take a look the repository of pgweb.

I done some fixes and propose refactor changes but the main idea of
theses patches is start to build a test suite for project in Django.

There a two main patches with tests

- 0004-Refactor-Move-the-view-for-Robots-inside-a-text-file.patch
- 0005-Refactor-Remove-extra-else-in-__str__-for-Quote.patch

With that could build more test to maybe to help the deploy and CI
process (I don't know if there any).

--
Rodrigo Ramírez Norambuena
http://www.rodrigoramirez.com/

Hi Rodrigo,

Thank you for working on this, this is very exciting!

Comments inline:

On 9/9/19 5:21 PM, Rodrigo Ramírez Norambuena wrote:
> Hello everyone!
>
> I've take a look the repository of pgweb.
>
> I done some fixes and propose refactor changes but the main idea of
> theses patches is start to build a test suite for project in Django.

I have added patch 0001.

With patch 0002, I don't know if we have anything in production that
utilizes "loaddata" and as such am hesitant to update the
requirements.txt file.

Can you please explain why you have allowed hosts set like that for
0003? I have not needed to configure my development copy of pgweb to
have that.

> There a two main patches with tests
>
> - 0004-Refactor-Move-the-view-for-Robots-inside-a-text-file.patch
> - 0005-Refactor-Remove-extra-else-in-__str__-for-Quote.patch
>
> With that could build more test to maybe to help the deploy and CI
> process (I don't know if there any).

This is definitely a good start given we should really be adding more
tests to pgweb in general. This does not need some work.

For 0004, first, I would not turn the current "robots" view function
into a TemplateView. We've not added class-based views (outside of the
RedirectView) into the codebase. Discussions about adding CBVs have
generally yielded to keep the current method of function based views. As
this should not affect the tests, I would advise leaving the robots.txt
generation view as is.

I'm not opposed to moving the robots.txt to its own file (similar to
what we do with the HTML templates), but also not seeing an immediate
need either.

(Also please leave the documentation around that function in place :) I
don't feel that the tests should replace the documentation around the code.)

The test case itself looks fine. I would suggest we maybe pick a
filename / structure that makes it clear that these are tests for the views.

For 0005, I would not put that in a migration. I would put that in a
test runner or whatever script we will use to build the test
environment, to mock varnish (if we need to -- not sure if we do?).

Similarly, I think we should decide on how we want to store our tests to
differentiate between models / views. In the past, I've had
subdirectories in my test folders to distinguish this, but IIRC it
requires a custom testrunner.

I would be fine with accepting the change to the if/else on its own, as
it eliminates the superfluous "else" if there is no opposition to that.

Thanks! Looking forward to seeing testing in the pgweb codebase :)

Jonathan

On Wed, Sep 11, 2019 at 4:05 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> Hi Rodrigo,
>
> Thank you for working on this, this is very exciting!

:)
> With patch 0002, I don't know if we have anything in production that
> utilizes "loaddata" and as such am hesitant to update the
> requirements.txt file.


For that, I send a new patch to replace. I think is a better way to
handle the dependencies, because in test environment we could need
additional modules
(0002-Split-requirement.txt-dependencies.patch)

>
> Can you please explain why you have allowed hosts set like that for
> 0003? I have not needed to configure my development copy of pgweb to
> have that.

If you are working a different host (virtual-host, external IP, etc..)
browser testing is against not localhost. The ALLOWED_HOST in '*' can
resolve this trouble

> > There a two main patches with tests
> >
> > - 0004-Refactor-Move-the-view-for-Robots-inside-a-text-file.patch
> > - 0005-Refactor-Remove-extra-else-in-__str__-for-Quote.patch
> >
> > With that could build more test to maybe to help the deploy and CI
> > process (I don't know if there any).
>
> This is definitely a good start given we should really be adding more
> tests to pgweb in general. This does not need some work.
>
> [...]
>
> (Also please leave the documentation around that function in place :) I
> don't feel that the tests should replace the documentation around the code.)
>
> The test case itself looks fine. I would suggest we maybe pick a
> filename / structure that makes it clear that these are tests for the views.

For this, add the patch 0005-Add-tests-for-robots-core-view.patch


> For 0005, I would not put that in a migration. I would put that in a
> test runner or whatever script we will use to build the test
> environment, to mock varnish (if we need to -- not sure if we do?).

Mock the varnish in first step could be more obfuscate because a
purge_urls is present in the almost all models. The kick start is a
add sql into db as varnish_local, after that could add a helper to
test varnish function to load these functions in db.

About not put in migration seams good choice but this migration run
only in test model. I'll could find a other way to add for the runner
or more clean. If you have any idea about this let me know.

> Similarly, I think we should decide on how we want to store our tests to
> differentiate between models / views. In the past, I've had
> subdirectories in my test folders to distinguish this, but IIRC it
> requires a custom testrunner.

Ahm. i think we should keep the struct app/tests/test_{models, views, admin}.py

>
> I would be fine with accepting the change to the if/else on its own, as
> it eliminates the superfluous "else" if there is no opposition to that.
>
> Thanks! Looking forward to seeing testing in the pgweb codebase :)

Your welcome!


--
Rodrigo Ramírez Norambuena
http://www.rodrigoramirez.com/

On Thu, Sep 12, 2019 at 12:25 PM Rodrigo Ramírez Norambuena
<decipher.hk@gmail.com> wrote:
>
> On Wed, Sep 11, 2019 at 4:05 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> >

>
>
> > For 0005, I would not put that in a migration. I would put that in a
> > test runner or whatever script we will use to build the test
> > environment, to mock varnish (if we need to -- not sure if we do?).
>
> Mock the varnish in first step could be more obfuscate because a
> purge_urls is present in the almost all models. The kick start is a
> add sql into db as varnish_local, after that could add a helper to
> test varnish function to load these functions in db.
>
> About not put in migration seams good choice but this migration run
> only in test model. I'll could find a other way to add for the runner
> or more clean. If you have any idea about this let me know.


For this topic, I attach a new patch with a runner. Inside the runner
is execute SQL sentences for Varnish.

This is better approach against to previous inside of migration model.

Regards!
--
Rodrigo Ramírez Norambuena
http://www.rodrigoramirez.com/

On Sun, Sep 15, 2019 at 2:34 PM Rodrigo Ramírez Norambuena
<decipher.hk@gmail.com> wrote:
>
> On Thu, Sep 12, 2019 at 12:25 PM Rodrigo Ramírez Norambuena
> <decipher.hk@gmail.com> wrote:
> >
> > On Wed, Sep 11, 2019 at 4:05 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> > >
>
> >
> >
>
>
> For this topic, I attach a new patch with a runner. Inside the runner
> is execute SQL sentences for Varnish.
>
> This is better approach against to previous inside of migration model.
>

Hi Jonathan,

There something  more we'll need to change for these patches?

--
Rodrigo Ramírez Norambuena
http://www.rodrigoramirez.com/

On Sun, Sep 15, 2019 at 2:34 PM Rodrigo Ramírez Norambuena
<decipher.hk@gmail.com> wrote:
>
> On Thu, Sep 12, 2019 at 12:25 PM Rodrigo Ramírez Norambuena
> <decipher.hk@gmail.com> wrote:
> >
> > On Wed, Sep 11, 2019 at 4:05 PM Jonathan S. Katz <jkatz@postgresql.org> wrote:
> > >
>
> >
> >
> > > For 0005, I would not put that in a migration. I would put that in a
> > > test runner or whatever script we will use to build the test
> > > environment, to mock varnish (if we need to -- not sure if we do?).
> >
> > Mock the varnish in first step could be more obfuscate because a
> > purge_urls is present in the almost all models. The kick start is a
> > add sql into db as varnish_local, after that could add a helper to
> > test varnish function to load these functions in db.
> >
> > About not put in migration seams good choice but this migration run
> > only in test model. I'll could find a other way to add for the runner
> > or more clean. If you have any idea about this let me know.
>
>
> For this topic, I attach a new patch with a runner. Inside the runner
> is execute SQL sentences for Varnish.
>
> This is better approach against to previous inside of migration model.


Hey Guys!,

There someone can take a look to my previous patches?

I want to introduce some changes to build a test suite but I need
address with the before patches.

Regards!



--
Rodrigo Ramírez Norambuena
http://www.rodrigoramirez.com/