⚡️ Improve backend test performance by patching password hashing

[saltie2193]

Improve performance of backend tests by patching password hashing during tests.

NOTE: To reduce interference with preexisting data, all existing users and items are now deleted before the tests are run.
This should not be noticable outside of testing, since all users and items already have been deleted after testing anyway.

[mdrxy]

This substantially reduces testing time - thank you!

Only thing I noticed is that session.execute should become session.exec due to deprecation.

[saltie2193]

At least according to mypy, session.exec of a SQLModel Session does not have an overload that would allow for passing a delete statement. Using it will fail the linting.
However it seems to work anyway.

mypy output when using session.exec:

app/tests/conftest.py:28: error: No overload variant of "exec" of "Session" matches argument type "Delete"  [call-overload]
app/tests/conftest.py:28: note: Possible overload variants:
app/tests/conftest.py:28: note:     def [_TSelectParam] exec(self, statement: Select[_TSelectParam], *, params: Mapping[str, Any] | Sequence[Mapping[str, Any]] | None = ..., execution_options: Mapping[str, Any] = ..., bind_arguments: dict[str, Any] | None = ..., _parent_execute_state: Any | None = ..., _add_event: Any | None = ...) -> TupleResult[_TSelectParam]
app/tests/conftest.py:28: note:     def [_TSelectParam] exec(self, statement: SelectOfScalar[_TSelectParam], *, params: Mapping[str, Any] | Sequence[Mapping[str, Any]] | None = ..., execution_options: Mapping[str, Any] = ..., bind_arguments: dict[str, Any] | None = ..., _parent_execute_state: Any | None = ..., _add_event: Any | None = ...) -> ScalarResult[_TSelectParam]
app/tests/conftest.py:29: error: No overload variant of "exec" of "Session" matches argument type "Delete"  [call-overload]
app/tests/conftest.py:29: note: Possible overload variants:
app/tests/conftest.py:29: note:     def [_TSelectParam] exec(self, statement: Select[_TSelectParam], *, params: Mapping[str, Any] | Sequence[Mapping[str, Any]] | None = ..., execution_options: Mapping[str, Any] = ..., bind_arguments: dict[str, Any] | None = ..., _parent_execute_state: Any | None = ..., _add_event: Any | None = ...) -> TupleResult[_TSelectParam]
app/tests/conftest.py:29: note:     def [_TSelectParam] exec(self, statement: SelectOfScalar[_TSelectParam], *, params: Mapping[str, Any] | Sequence[Mapping[str, Any]] | None = ..., execution_options: Mapping[str, Any] = ..., bind_arguments: dict[str, Any] | None = ..., _parent_execute_state: Any | None = ..., _add_event: Any | None = ...) -> ScalarResult[_TSelectParam]
Found 2 errors in 1 file (checked 38 source files)

I'm not sure what's the better option here. Do we ignore the type checker and provided signature for session.exec? Or, do we insist on not using deprecated methods were it seems like there is no replacement?

[YuriiMotov]

@saltie2193, good idea!

In addition to the comment below, I think we should leave 1 test that would be run without this patching to ensure real algorithm works (mark test with @pytest.mark.noautofixt and pass db fixture explicitly).

[YuriiMotov]

I think we shouldn't mix disable_password_hashing with db fixture

[saltie2193]

In this case should we make disable_password_hashing an autouse fixture and just explicitly disable the fixture in tests where we want hashing to be enabled?

I think this would still be necessary since I would assume we want hashing disabled for almost all tests and only enable it for a few specific ones.

[YuriiMotov]

Yes

[saltie2193]

After some investigation the disable_password_hashing and db fixtures were mixed because of the call to init_db in the db fixture. When password hashing is not enabled during this setup, the database is initialized with a hashed password of the superuser, while the tests try to authenticate the user without hashing the provided password.
This could be mitigated if tests would not rely on an initial setup of the database and the configured superuser, but setup up (and tear down) the required test data on their own. I don't think this easily fixable here, since we would need to remove the dependency of the tests to an initial setup.

For the time being while this interdependency between test exists, a workaround or solution could be to create a new module testing authentication with hashing enabled and create overrides for db and client with enabled password hashing, recreating the database with hashed passwords. In this case the default db and client fixtures would stay mixed with the disable_password_hashing fixture, since we want to disable it by default in almost every test case and it is required due to the dependency on the initial setup.

[YuriiMotov]

Not sure I understand the problem..
What if we create init_db_fixtrue fixture:

@pytest.fixture(scope="session", autouse=True)
def init_db_fixture(db: Session) -> Generator[None, None, None]:
    init_db(db)
    yield

.. and remove the init_db call from db fixture?

The main question is: if the order of fixtures in conftest.py defines the order they will be applied automatically?

[saltie2193]

The problem is the password patching needs to be applied, before the init_db_fixture is executed. Every time we want to disable or enable it, the database needs to be initialized again.

As far as I understand the pytest documentation, only factors are

1. scope,
2. dependencies, or
3. autouse.

For the time being it seems to work, but the dependency of tests to an initial setup is still a problem. The current compromise is to introduce a marker to enable password hashing on a module level. So the initial setup is only run before every module. In the long term I think it would be best to remove this dependency allowing to enable or disable hashing per test. This would remove the problem of execution order of the fixtures as well.

[YuriiMotov]

Sorry for pushing to your branch, it was simpler than explaining this in review..
If you don't like changes, feel free to revert commits or use only part of suggested changes.

I suggest simplify tests:

• Since we only patch one module, we don't need that complex things with ExitStack.
• Added if..else condition in disable_password_hashing to make logic clearer
• Explicitly added disable_password_hashing dependency to db fixture
• Removed init_db_fixture (moved init logic to db fixture)
• Simplified docstrings and comments (IMO, some of them were too verbose, some were incorrect)
• Removed unnecessary second test from test_login_with_hashing.py

[github-actions]

This pull request has a merge conflict that needs to be resolved.

[saltie2193]

That's basically the original proposal now so looks fine to me.

I hope it's fine I reverted the changes to keep the patch_passwors_hashing utility. I think this way it's a bit cleaner and it can be reused easily. If you feel strong about not having it feel free to remove it again.

[YuriiMotov]

LGTM!

This disables password hashing for all tests except one (in test_login_with_hashing.py - added to make sure login functionality still works with password hashing).

I still think it would be simpler to just use two patches instead of implementing patch_password_hashing with ExitStack, but let's Sebastian decide. I would revert this commit

@saltie2193, thank you for working on this!

[YuriiMotov]

Suggested change

	Contextmanager to patch ``pwd_context`` in the given modules.
	:param modules: list of modules to patch.
	:return:
	Contextmanager to patch `pwd_context` in the given modules.

We don't use this style for docstrings in this project