Issue/14 & issue/15 added tests to the dispatcher service #patch

[timberhill]

Closes #14, #15

[danyloid]

@timberhill unfortunately test run fails on Windows, adding the log

PS C:\Storage\development\flam-flam\dispatcher-service> make test        
"..... Installing depencencies"
"..... Linting"
process_begin: CreateProcess(NULL, flake8, ...) failed.
make (e=2): The system cannot find the file specified.
make: *** [Makefile:16: lint] Error 2

[danyloid]

That might be an environment issue though, looking into it

[danyloid]

python -m pytest works for me, apparently pytest is not added to %PATH% on Windows.

[timberhill]

Ah, noted. Hold on

[danyloid]

One more thing - .env file is not automatically fetched when running pytest, I'll likely need extra tooling to load variables.

[timberhill]

@danyloid yup, just fixed that. Check if that works now

[danyloid]

Makefile works well now, thanks
Still need a way to set environment variables for the script - currently looking at the options for Windows

[danyloid]

Hm, based on a quick search I was not able to find a generic solution :/

[danyloid]

That being said, do we actually want dispatcher to connect to Reddit during the test run ?

[timberhill]

So does it not work with the include and export?

[danyloid]

Does not seem to.

This is what I'm getting

FAILED tests/test_dispatcher.py::test_init - asyncpraw.exceptions.MissingRequiredAttributeException: Required configuration setting 'client_id' missing.
FAILED tests/test_dispatcher.py::test_config - asyncpraw.exceptions.MissingRequiredAttributeException: Required configuration setting 'client_id' missing.
FAILED tests/test_dispatcher.py::test_dispatch_comments - asyncpraw.exceptions.MissingRequiredAttributeException: Required configuration setting 'client_id' missing.
FAILED tests/test_dispatcher.py::test_dispatch_submissions - asyncpraw.exceptions.MissingRequiredAttributeException: Required configuration setting 'client_id' missing.

[timberhill]

How do you normally load the .env file into the environment variables though? Or do you just do that in docker?

[danyloid]

I don't 😄

I'm used to use .env files with web Node / Web projects, courtesy of dotenv.
Docker offers support for the .env files.
For .NET - there is a build in expand mechanism for appsettings.json based on the path in the JSON.

All of the cases above are tooling specific - I haven't been able to find a generic way running a script with .env preloaded.
There is a way to set the variables inline for the script execution, however was not able to find a way to load it from .env file.

Granted - I could just set these in system, instead of attempting to use env file, however that is not a clean way to do it.
Alternatively I could use WSL2 I guess.

The question however still stands:

That being said, do we actually want dispatcher to connect to Reddit during the test run ?

I get that in this case the server behavior is predictable, however I'm not used to have extra dependencies in the test execution as a rule of a thumb + we don't really need to test asyncpraw, do we ?

[danyloid]

Nevermind the .env issue - I'm getting used to having to do extra moves due to being on Windows at this point :)
I'll probably do a scripts to load the variables from .env files & clean up after the execution.

[timberhill]

In CLI I can also run this: set -a; source .env && python -m pytest. But that looks shell specific. Git client on windows installs a bash shell with it if I remember correctly - I have used that a lot just because ✨ bash ✨. Wonder if that is easier and more of a lightweight solution to this compared to WSL2. Doing extra steps is suboptimal however. We can put the tst into a docker container so that is it actually cross platform.

do we actually want dispatcher to connect to Reddit during the test run ?

That is a gooooood question. The issue with the dispatcher is, there isn't much unit testing to do, but there are some options:

1. We can mock Reddit API and make asyncpraw package talk to that. This would have to be a written ourselves, unless I have missed a good out of the box mocker.
2. We mock the asyncpraw package itself by creating one with equivalent functionality (at least what we use) and use that for testing
3. We don't actually test the dispatcher.

Making tests depend on an external service is definitely an antipattern, I do understand that. This is why the comment and submission services are mocked - that was simple. But at the same time it's a simple solution for the problem and we can build on that if needed. I have created separate creds for the CI, so there's at least that extremely small consolation.

At the same time, I might be missing something here, so please let me know if I did!

[danyloid]

@timberhill you are correct - I'll give the git shell a try, thanks 😄

Regarding the testing - I'm kind of used to go with option 2. Generally I'm trying to avoid testing 3d parties, unless there is a specific reason not to (e.g. hard to mock).
Unless we're doing e2e tests, validating scenarios across the whole solution, which I believe is not the goal here.

[timberhill]

It is sort of an e2e test for this one particular service though :D

But good point about option2, thank you - will have a look if I can utilise python mock for this easily.

[danyloid]

Yes, I get it, but it can also introduce unnecessary flakiness for test executions.
Especially when the external integrations are less predictable (which usually is the case)

[danyloid]

@timberhill Git Bash seems to leverage WSL2 now, since I have enabled it.
That being said - I browsed the README, is there a python version requirement ?

$ make test
..... Installing depencencies
..... Linting
..... Running tests
============================= test session starts =============================
platform win32 -- Python 3.10.10, pytest-7.2.2, pluggy-1.0.0
rootdir: C:\Storage\development\flam-flam\dispatcher-service
plugins: anyio-3.6.1, asyncio-0.21.0, requests-mock-1.10.0
asyncio: mode=strict
collected 5 items

tests\test_dispatcher.py ...FF                                           [100%]

================================== FAILURES ===================================
___________________________ test_dispatch_comments ____________________________

    @pytest.mark.asyncio
    async def test_dispatch_comments() -> None:
        from app import RedditDispatcher
        dispatcher = RedditDispatcher(**TEST_CONFIG)
        with requests_mock.Mocker() as m:
            m.get(TEST_CONFIG["comment_endpoint"], text="ok")
            try:
>               with timeout(TIMEOUT, RuntimeError):

tests\test_dispatcher.py:57:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\__init__.py:148: in __enter__
    return super(Timeout, self).__enter__()
C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\compat.py:14: in __enter__
    return self.gen.__next__()
C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\__init__.py:97: in timeout
    set_sighandler()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

    def set_sighandler():
>       current = signal.getsignal(signal.SIGALRM)
E       AttributeError: module 'signal' has no attribute 'SIGALRM'. Did you mean: 'SIGABRT'?

C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\__init__.py:77: AttributeError
------------------------------ Captured log call ------------------------------
INFO     dispatcher:dispatcher.py:22 Set submission endpoint to http://test:8080
INFO     dispatcher:dispatcher.py:27 Set comment endpoint to http://test:8080
INFO     dispatcher:dispatcher.py:31 Watching subreddits ['test']
__________________________ test_dispatch_submissions __________________________

    @pytest.mark.asyncio
    async def test_dispatch_submissions() -> None:
        from app import RedditDispatcher
        dispatcher = RedditDispatcher(**TEST_CONFIG)
        with requests_mock.Mocker() as m:
            m.get(TEST_CONFIG["submission_endpoint"], text="ok")
            try:
>               with timeout(TIMEOUT, RuntimeError):

tests\test_dispatcher.py:72:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\__init__.py:148: in __enter__
    return super(Timeout, self).__enter__()
C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\compat.py:14: in __enter__
    return self.gen.__next__()
C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\__init__.py:97: in timeout
    set_sighandler()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

    def set_sighandler():
>       current = signal.getsignal(signal.SIGALRM)
E       AttributeError: module 'signal' has no attribute 'SIGALRM'. Did you mean: 'SIGABRT'?

C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\__init__.py:77: AttributeError
------------------------------ Captured log call ------------------------------
INFO     dispatcher:dispatcher.py:22 Set submission endpoint to http://test:8080
INFO     dispatcher:dispatcher.py:27 Set comment endpoint to http://test:8080
INFO     dispatcher:dispatcher.py:31 Watching subreddits ['test']
============================== warnings summary ===============================
tests/test_dispatcher.py::test_dispatch_comments
tests/test_dispatcher.py::test_dispatch_submissions
  C:\Users\danyl\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.10_qbz5n2kfra8p0\LocalCache\local-packages\Python310\site-packages\interruptingcow\__init__.py:90: DeprecationWarning: currentThread() is deprecated, use current_thread() instead
    if threading.currentThread().name != 'MainThread':

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ===========================
FAILED tests/test_dispatcher.py::test_dispatch_comments - AttributeError: mod...
FAILED tests/test_dispatcher.py::test_dispatch_submissions - AttributeError: ...
=================== 2 failed, 3 passed, 2 warnings in 0.61s ===================
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x0000019253E60D60>
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x0000019253F20F40>
make: *** [Makefile:21: test] Error 1

I'm on 3.10

$ python --version
Python 3.10.10

[danyloid]

NVM, that is Windows too, I've got confused with the UI
Here is the rc: https://stackoverflow.com/questions/47545002/python-standard-lib-signal-attributeerror-module-signal-has-no-attribute

[danyloid]

Ok, so WSL2 is the way to do it.

danyloid@danyloid-xps:/mnt/c/Storage/development/flam-flam/dispatcher-service$ make test
..... Installing depencencies
..... Linting
..... Running tests
================================================= test session starts ==================================================
platform linux -- Python 3.10.6, pytest-7.2.2, pluggy-1.0.0
rootdir: /mnt/c/Storage/development/flam-flam/dispatcher-service
plugins: requests-mock-1.10.0, asyncio-0.21.0
asyncio: mode=strict
collected 5 items

tests/test_dispatcher.py ....

[timberhill]

Also good point about the python version! It is set in the dockerfile, but I think I use a different one locally. Will have a look.

[danyloid]

Right & we probably need to specify in the README that the dispatcher service requires Linux to run.

[timberhill]

Should be good for review now. The package that talks to Reddit has been mocked - not sure if this is the best way of doing this, but it seems to work.
@danyloid 👀

[danyloid]

@timberhill looks good overall, but since I am not familiar with Python, I would appreciate a bit more elaborate description outlining the approach😄

[danyloid]

LGTM, have 2 suggestions though:

1. Mocks do not offer option to validate usage declaratively.

I've noticed you essentially have created mocks manually that match the contract that is used in dispatcher, which does make sense. However it does not give you an option to validate calls to specific components if needed.

Would it make sense to use something like https://docs.python.org/3/library/unittest.mock.html ?

That way you also could validate specific calls and parameters passed in test body, rather than having assertions within mock. E.g. via assert_called_with(3, 4, 5, key='value')

2. Request is not validated

Are you validating that the requests were properly initiated for generated comments / submissions ?

Currently requests are mocked, but let's say the dispatcher gets broken in the futured and some data is skipped for whichever reason - this test would likely still succeed, would it not ?

Nor is the request format validated - hence the tests might not catch unintended change.

Unfortunately I need to do a bit of homework to suggest the changes to the codebase, so I'll go do that :)

[danyloid]

Hey, I found this: https://requests-mock.readthedocs.io/en/latest/history.html
Now, if the data streamed from SubredditStream mock was defined in the test file and passed into Reddit mock, or generated in the way it is generated now & stored within Reddit mock in a way that would make it accessible from test, we could actually validate that request for each dispatched comment / submission has been generated & match the data.

[timberhill]

Okay, here we are. I have removed most of the previous manual mock and replaced it with python Mock.

The only thing that was giving me trouble was async iteration that I think cannot be reasonably mocked via python AsyncMock class. Therefore I have left the SubmissionStream class in.

Big changes in this whole pull request:

1. Dispatcher does not create its own AsyncPRAW instance, but takes it as an argument. Authentication is now handled in the __main__ function, leaving only dispatching to the dispatcher.
2. Mock AsyncPRAW instance is create in the pytest fixture, which is the correct way of doing this.
3. SubmissionStream class contains predefined sets of objects it can dispatch, which is then compared to the objects that were received by the mock endpoints.
4. The files are now linted with the default flake8 config

[timberhill]

@danyloid please tear this apart :D
Seriously though, it looks okay to me, but let me know if there is anything that you can see could be made better!

[danyloid]

@timberhill looks good to me, thanks for making the changes.