Tests: Fixture implementation duplicated between deprecated manage/tests/pytest_fixtures.py and tools/pytest_fixtures/

[GeigerJ2]

For instance, aiida_localhost in manage/tests/pytest_fixtures.py:

aiida-core/src/aiida/manage/tests/pytest_fixtures.py

Lines 657 to 668 in dd866ce

	@pytest.fixture(scope='function')
	def aiida_localhost(aiida_computer_local) -> Computer:
	"""Return a :class:`aiida.orm.computers.Computer` instance representing localhost with ``core.local`` transport.
	Usage::
	def test(aiida_localhost):
	assert aiida_localhost.transport_type == 'core.local'
	:return: The computer.
	"""
	return aiida_computer_local(label='localhost')

and in tools/pytest_fixtures/orm.py:

aiida-core/src/aiida/tools/pytest_fixtures/orm.py

Lines 193 to 204 in dd866ce

	@pytest.fixture
	def aiida_localhost(aiida_computer_local) -> 'Computer':
	"""Return a :class:`aiida.orm.computers.Computer` instance representing localhost with ``core.local`` transport.
	Usage::
	def test(aiida_localhost):
	assert aiida_localhost.transport_type == 'core.local'
	:return: The computer.
	"""
	return aiida_computer_local(label='localhost')

with manage/tests/pytest_fixtures.py being deprecated:

aiida-core/src/aiida/manage/tests/pytest_fixtures.py

Lines 54 to 57 in dd866ce

	warn_deprecation(
	'The module `aiida.manage.tests.pytest_fixtures` is deprecated, please use `aiida.tools.pytest_fixtures` instead.',
	version=3,
	)

Of course, we still need to keep access from the old module for now for backwards compatibility, otherwise plugins break, e.g.:

https://github.com/aiidateam/aiida-quantumespresso/blob/57e7463c5727775d6a0470a41d1aca0ec4083b9a/tests/conftest.py#L13

But rather than duplicating the source code as it seems to be done now, we should let the fixtures in manage/tests/pytest_fixtures.py call the actual implementation in tools/pytest_fixtures/orm.py. Pinging the aiida-core office @agoscinski, @unkcpz, @khsrali.

[unkcpz]

I totally agree. I think the doc page https://aiida.readthedocs.io/projects/aiida-core/en/stable/topics/plugins.html#plugin-test-fixtures Is very helpful to show what fixtures that plugin developers can use. We may also want to add same paper for public APIs.

[danielhollas]

I honestly don't know if it is worth the churn to be refactoring deprecated functions. I think it is easier to let them be and eventually remove them, and only do new development in the new ones.

[unkcpz]

I guess the problem @GeigerJ2 and @khsrali had today was they made changes to new fixtures and then found changes need to apply to the deprecated one as well, correct me if I am wrong.
The problem is if it is marked as "will be deprecated", do we still need to keep it maintained? The change by pointing to only single definition sounds a good solution for me. I don't have too much experience on how to deprecate things, aiida_localhost sound like very small so it can rarely break thus may not need to change. But point to one definition can be a good practice if it is not too hard?

[agoscinski]

I don't have too much experience on how to deprecate things

We don't have a consistent way to do it over the modules but in this case manage/tests/pytest_fixtures is anyway not public API, so we can just remove it.

[GeigerJ2]

I honestly don't know if it is worth the churn to be refactoring deprecated functions. I think it is easier to let them be and eventually remove them, and only do new development in the new ones.

Yeah, I agree, that's why I only made the issue so far, not a PR :D Though, it should be quite quick. Just remove the source code there and import from the more recent modules instead.

I guess the problem @GeigerJ2 and @khsrali had today was they made changes to new fixtures and then found changes need to apply to the deprecated one as well, correct me if I am wrong.

Not really changing anything, but I wanted to look at the fixture implementation, and global search in VS Code yielded two definitions, which confused me.

[danielhollas]

We don't have a consistent way to do it over the modules but in this case manage/tests/pytest_fixtures is anyway not public API, so we can just remove it.

Regardless of a definition of a public API, these fixtures are widely used in the AiiDA ecosystem, so we shouldn't remove them just yet IMO, not until a few releases. The new fixtures were only introduced in v2.6 so it'll be a while until everybody catches up.

[danielhollas]

Yeah, I agree, that's why I only made the issue so far, not a PR :D

👍 😄

Though, it should be quite quick. Just remove the source code there and import from the more recent modules instead.

To clarify, my main concern was not really about the time to implement it, but the fact that you would be introducing coupling between the manage and tools module, which I think is not desireable. Also, once you have a single source of truth, if you would change it, it would change the old fixtures as well, which might not be desireable and could break people.