Init tests

[ovv]

No description provided.

[ovv]

 /.../pyslackers/website/tests/test_website.py:17: DeprecationWarning: Changing state of started or joined application is deprecated
    client.app["website_app"]["slack_client"] = slack_client

We get a warning during the tests. I'm not yet sure what's the best way to fix that. It should be ok for now

[mattrasband]

I think we are going a direction that may be more brittle and targeted than we need for a small site like this.

1. We should mostly focus on the integration tests and allow the app to use both postgres and redis (for now). The FakeRedis implementation scares me and adds maintenance if/when we expand redis usage, which doesn't seem too valuable to me.
2. Let's pull out the fetch logic from .web.tasks from the processing logic, that allows easy mocking to avoid needing real data but also avoid changing function signatures strictly for tests.
3. We can document how to run tests utilizing the provided docker-compose.yml instead of adding a mostly duplicate one.

A follow up for me (or whoever wants to do it): Let's model the remote objects instead of passing dicts around.

[mattrasband]

These dependencies are already met by docker-compose.yml. Can you expand on why we need a dedicated test one?

[ovv]

docker-compose.yml bring dependencies + website up for testing and only open 8000 to show the website. docker-compose.test.yml goal is to only spin up docker + redis so you have a local instances the tests can connect to.

There may be a better way to do that

[mattrasband]

The way I would probably run it is:

docker-compose run web python -m pytest tests/

It may be worth throwing in a makefile to simplify commands (see #374)

[mattrasband]

This could be simplified to: validate=bool since it's a callable

[mattrasband]

To avoid confusion with the routing (as in L22), why not make a base test_client for the top-level app, so all the HTTP calls are as they would look in the browser. We can still have the website_client customize the slack_client it uses for testing.

[ovv]

I don't understand what you mean.
L22 check that / is correctly redirect to /web as it is in production.

We only have one subapp for the moment so we could have one app fixture doing everything but once we have multiple subapps we may need to split the fixture anyways.

[mattrasband]

The confusion I mean is that it's odd that the "website app client" can GET / since that is a route on the main "app" and from my view it shouldn't be reachable.

I agree with breaking them up, I would expect us to have a app_client, web_client and *_client (sirbot or whatever else we add).

But I can refactor some ideas after we merge it in. I typically namespace the tests to match the packages under test.

[mattrasband]

I really do not want to test logging implementation, that seems brittle and not like something to ensure doesn't change.

[ovv]

The goal is not to check the logging implementation but to ensure not exceptions where raised. It kinds of loop back to the return / raise issue of the tasks.

IMO it's fine for now.

Later on, we should split those tasks into two functions. One handling the logging / exception catching for apscheduler and a second doing the actual work that we can test

[ovv]

I think we are going a direction that may be more brittle and targeted than we need for a small site like this.

1. We should mostly focus on the integration tests and allow the app to use both postgres and redis (for now). The `FakeRedis` implementation scares me and adds maintenance if/when we expand redis usage, which doesn't seem too valuable to me.

2. Let's pull out the fetch logic from `.web.tasks` from the processing logic, that allows easy mocking to avoid needing real data but also avoid changing function signatures strictly for tests.

3. We can document how to run tests utilizing the provided `docker-compose.yml` instead of adding a mostly duplicate one.

A follow up for me (or whoever wants to do it): Let's model the remote objects instead of passing dicts around.

1. Agree
2. Agree but that requires a lot more work (postpone for another PR ?)
3. As I don't use docker that much I'm not sure what is the recommended way. If we enforce the need for postresql + redis for the tests I'm sure there is a way to parametrize the docker-compose.yml to run pytest instead of the webserver. With tox integration if possible.

[ovv]

@mrasband updated, please take another look