elixir-cloud-aai / foca

Opinionated Flask microservice archetype for quick OpenAPI-based microservice development
Apache License 2.0
16 stars 12 forks source link

fix: failing unit tests #214

Closed athith-g closed 6 months ago

athith-g commented 6 months ago

Issue 23 unit tests are failing. 22 tests fail due to connecting to the wrong db port here and here (should be using 27017).

Reproduce Run the following commands:

  1. pip install -r requirements.txt
  2. pip install -r requirements_dev.txt
  3. cd examples/petstore-access-control
  4. docker compose up -d
  5. cd ../..
  6. pytest tests
uniqueg commented 6 months ago

Can you please tell me where you had the tests failing? The CI pipeline seems to work fine (now that #218 was fixed).

athith-g commented 6 months ago

Sorry I should have been more descriptive. The test in tests/security/test_cors.py is the only one that doesn't fail due to server timeouts. Tests in tests/security/access_control/test_access_control_server.py, tests/security/access_control/test_register_access_control.py, and tests/security/access_control/foca_casbin_adapter/test_adapter.py all fail due to timeouts.

Screenshot 2024-05-19 at 6 45 08 PM

If the tests are passing in the pipeline I may be doing something incorrectly. Is there a step missing in the reproduction steps?

uniqueg commented 6 months ago

Yes, I guess so. It appears that when the unit tests for Casbin access control stuff were written in FOCA, the connections to the database were not mocked. So the tests require a database to be up and running (you can also see that in the GitHub Actions workflow, it uses the mongodb-github-action).

To make the tests pass locally, I ran the following:

docker run --name mongodb -p 12345:27017 -d mongo:7.0

In my opinion, these are integration tests rather. Unit tests shouldn't have requirements like that. But I guess it's easy enough to fulfull the requirements, so I am not planning on changing it now.

Btw, I guess your fix in #215 worked for you because you probably happened to conincidentally have a MongoDB instance called mongodb running at port 27017 :)

uniqueg commented 6 months ago

Anyway, if you can confirm that this works for you (i.e., firing up a MongoDB instance at port 12345), then I think you can close this issue and the accompanying PR :)

athith-g commented 6 months ago

That makes sense thank you for clearing things up!

Before I close the issue, I think the changes to tests/security/test_cors.py (https://github.com/elixir-cloud-aai/foca/pull/215/commits/15ba28638ae22a44c8fd32561df5a0d562bbba5f) may still be valid. The reason it passes in the pipeline and not in my local environment is that the error only shows up in python 3.12. Even though the test passes in python 3.10, I don't think it's testing the desired functionality (i.e., the assertion is true even though the mock object wasn't called), as described here.

uniqueg commented 6 months ago

Thanks for pointing this out, good catch! :+1: But I think I have already dressed this in b1100bcdb5a32d1b2062d5ecdaa9463a04a6782d, when I added support for Py3.12 last night. Can you please check and see if you agree on whether this fix addresses the underlying problem?

athith-g commented 6 months ago

You definitely did my bad! Everything looks good.