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/