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