deephdc / demo_app

A minimal toy application for demo and testing purposes.
MIT License
1 stars 2 forks source link

Testing framework #1

Open BorjaEst opened 1 year ago

BorjaEst commented 1 year ago

Test example is currently based in Unittesting framework. However, it is not starting any test client to simulate actual web replies (JSON with response codes).

Some of the WebAPI documentation frameworks (Flask, FastAPI) use pytest meanwhile others (Django) use unittest:

I am not sure what is the engine behind DEEPaaS so I am not sure which test client should I use.

Additionally I like testing with pytest (which is acually on the example test-requirements.txt). Are there any advantages on using unittesting vs pytest?

IgnacioHeredia commented 1 year ago

Hi @BorjaEst ,

I think unit testing inside modules is meant to test the module's functionalities themselves, not the access to those functionalities through the DEEPaaS API. For this DEEPaaS has it's own set of tests.

If you are interested, DEEPaaS is currently based in aiohttp, though this might change in the future.

Regarding to the testing framework (pytest, unittesting, etc), I guess it is up to the developer of that module to choose their preferred framework.

BorjaEst commented 1 year ago

Hi @IgnacioHeredia,

Thanks for the answer. I suppose then we have to make use of raising specific Exception for returning 40X values and other for 50X values? It should be enough to test that the defined API functions raises those. I like that idea as it makes developer's life easier.

However, I am not sure what would be the best way to test get_predict_args. Webargs is a bit tricky to configure and we might be changing web arguments properties ofter (or even comming form config file). How can I ensure that my created arguments will deserialize the way I want some inputs?

Here an example:

IgnacioHeredia commented 1 year ago

Again, I think testing deserialization tests are probably better located in deepaas test's, not in the individual module's test. It should be deepaas job to test that standard objects deserialize correctly.

IMHO modules' tests should test the functionalities of the modules, but without going throught deepaas, just calling the functions (predict, train, ect) directly. For this same reason, raised exceptions in the tests should be standard Python exceptions, not HTML exceptions.

Aside from this you can also decorate the api functions to translate standard exceptions to aiohttp exceptions to show errors more clearly to users. Note that this is unrelated to tests.

Maybe @vykozlov can contribute his opinion on this.

BorjaEst commented 9 months ago

Hello @IgnacioHeredia, I have some cases in where my tests are passing (so my api functions work) but the DEEPaaS does not start due to errors (i.e. if I create typo on the arguments functions).

Do you know of the best way to create a test that fails in case the DEEPaaS cannot start correctly? I would like to avoid to create a service at the ai4eosc platform with a container that is not able to start DEEPaaS.

IgnacioHeredia commented 9 months ago

Hi @BorjaEst ,

you mean that deepaas does not start if the is an error in get_train_args() for example, right? Can't you just test that function as well?

In any case, you might just create a test that launches deepaas with subprocess.Popen and check that doesn't throws an error.

import subprocess

command = (['deepaas-run',])
result = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
try:
    # Launch deepaas. As the process doesn't end by itself if successful,
    # let's give a 60s timeout to fail. After that consider it successful.
    output, error = result.communicate(timeout=60)
    if error or 'ERROR' in str(output):
        raise Exception('DEEPaaS failed to launch')
except subprocess.TimeoutExpired:
    pass

# 'success!'

I tested this fails when an error in get_args is present. I added the error 'ERROR' in str(output) condition because deepaas seems to not be piping well the errors (issue).

BorjaEst commented 9 months ago

Hi @IgnacioHeredia,

Yes! That is one of the possibilities. I could indeed test that, but complexity and amount of tests to maintain would increase. It feels a bit redundant to test a defined field.String is a field.String. Additionally, if by some reason aiohttp swagger does not accept the returned dict, the test will pass but the server will not start.

I like better your approach with subprocess, that way I might create an example that other users could just copy and paste.

Just one concern, the test will take 60 seconds to pass. I know it is because by default it loads the dummy model. Is there a way to call the python function that starts the service in way that fails if there are errors loading the model?

IgnacioHeredia commented 9 months ago

The 60s are not related to the dummy model. It's related to the fact that the deepaas process (if successful) never stops. So we have to define some stop point. If it arrives till that point without failing I consider the launch to be successful. I considered 60s to be enough to load a model and see if it fails, but that can probably be reduced (30s?). If the model fails earlier the test will take less time to pass.

BorjaEst commented 8 months ago

I do not like the idea of a timeout, the ideal would be that the call raises an exception if somethings goes wrong. Additionally, executing tests with subprocess might not be trivial.

In any case, I could not avoid the use of subprocess, oslo_config complains about no configuration being loaded but it seems there is no way to load configuration with oslo during runtime, which is a problem when parametrizing tests, see: https://github.com/pytest-dev/pytest/discussions/5461#discussioncomment-85530

For know I suppose the simplest is indeed subprocess call. I have modified your code a bit, as it does not "kill" the process after the test is finished:

@pytest.fixture(scope="module")
def deepaas_process(request, config_file):
    """Fixture to start deepaas process and kill it after tests."""
    with subprocess.Popen(
        args=["deepaas-run", "--config-file", config_file],
        stdout=subprocess.PIPE,  # Capture stdout
        stderr=subprocess.PIPE,  # Capture stderr
    ) as process:
        try:
            outs, errs = process.communicate(timeout=request.param)
        except TimeoutExpired:
            process.kill()
            outs, errs = process.communicate()
    return {"stdout": str(outs), "stderr": str(errs)}

@pytest.mark.parametrize("deepaas_process", [10], indirect=True)
def test_stdout_errors(deepaas_process):
    """Assert there are no errors in process stdout."""
    assert "ERROR" not in deepaas_process["stdout"]

@pytest.mark.parametrize("deepaas_process", [10], indirect=True)
def test_stderr_errors(deepaas_process):
    """Assert there are no errors in process stderr."""
    assert "ERROR" not in deepaas_process["stderr"]
IgnacioHeredia commented 8 months ago

@BorjaEst I'm afraid a timeout is conceptually unavoidable and thus needed, given that you never know when a module has finished loading.

For example, here are two usecases:

At min 3, you have no way to take apart those two usecases (neither has raised an Exception yet). At any given time, it is impossible to know if a module hasn't failed becaused it loaded successfully or because it is taking longer to load (and thus might fail afterwards). You could parse the logs to take two cases apart but that fix is even uglier I believe.

Anyway, glad you make it wotk for your usecase.