Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow reuse of test code in other projects #5068

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

sumpfralle
Copy link
Contributor

Description

Currently the test-related code of beets resides in the test directory, which is not part of the beets package.
Thus, external projects (in my case: mopidy-beets) cannot use any parts of beets' test infrastructure.

Specifically the TestHelper class (and indirectly some parts of test._common) may be useful for external projects (e.g. for working with an in-memory instance of a beets library).

This proposal consists of four commits:

  • remove import path mangling (it was not relevant anymore)
  • move reusable components from test.test_importer to test.helper
  • move TerminalImportSessionSetup from tests.test_ui_importer to test.helper
  • move reusable test-related modules (test._common and test.helper) into the beets package

These changes do not interfere with the current structure or behavior of the beets tests.

These changes would allow me to improve the tests of the mopidy-beets project.

@Serene-Arc Serene-Arc self-assigned this Jan 9, 2024
@sumpfralle
Copy link
Contributor Author

Any comments?

Or should I add a changelog entry (as mentioned by one of the CI checks) first?
(I was unsure whether this change justifies a changelog entry)

@Serene-Arc
Copy link
Contributor

I'm going to review this in full over the next few hours but I want to be clear that beets does not promise that these, or any, testing objects will remain static. These can and will be changed, so relying on them for your own project will be something you'll have to take on at your own risk and keep an eye on PRs here that might change those parts of our testing framework.

@Serene-Arc
Copy link
Contributor

If you could make sure that the tests pass with your changes, I will review after that.

@sumpfralle
Copy link
Contributor Author

If you could make sure that the tests pass with your changes, I will review after that.

Done. Thanks!

@sumpfralle
Copy link
Contributor Author

I'm going to review this in full over the next few hours but I want to be clear that beets does not promise that these, or any, testing objects will remain static. These can and will be changed, so relying on them for your own project will be something you'll have to take on at your own risk and keep an eye on PRs here that might change those parts of our testing framework.

Yes, I understand that the exposure of these components will make them part of the public interface of beets.

From my point of view, these pieces are only useful in testing environments. Thus, breaking these interfaces would not cause damage (only consuming time of developers). And users ("developers") probably understand, that such pieces of code are not bound to the same level of stability as the rest of beets' code.

But these are just my subjective assessments. I could perfectly understand, if you reach a different conclusion.

@Serene-Arc
Copy link
Contributor

Our CI testing is still failing. Are they working on your local machine?

@sumpfralle
Copy link
Contributor Author

Our CI testing is still failing. Are they working on your local machine?

Thanks for your reminder. Indeed the windows-CI failure was caused by my changes. This is fixed now.

The docs-related failure seems to be a recent problem of sphinx (fixed to an older version) combined with the latest sphinxcontrib-applehelp release. This is unrelated.

@sumpfralle
Copy link
Contributor Author

See #5080 for the "docs" failure.

@Serene-Arc
Copy link
Contributor

Could you please rebase this in light of the Sphinx changes being merged? That'll get rid of the failing CI and I can review in depth.

The import path mangling is not relevant (anymore?) for the two
ways of running tests:

* `python3 test/testall.py` (see CONTRIBUTING.rst):
  The `testall.py` script already adds the project path to `sys.path`.
* `tox -e py-cov`: this command is supposed to be run from the project
  path. Thus, the current directory is already the first of location
  in `sys.path`.

The previous mangling of the import path while loading a module could
lead to unwanted side-effects hidden in an unexpected location.
Instead, import path mangling should take place in the script being
called by the user (here: `testall.py`).
…lper`

`ImportHelper` and `AutotagStub` are used in many tests.
Thus, they should reside in a module which is obviously used by multiple
tests.
…` to `test.helper`

This class is imported by some other test modules.
Thus, it should reside in a module, which is obviously used by other
tests.
External Python packages interfacing beets may want to use an in-memory
beets library instance for testing beets-related code.
The `TestHelper` class is very helpful for this purpose.
Previously `TestHelper` was located in the `test/` directory.
Now it is part of `beets` itself (`beets.test.helper.TestHelper`) and
can be easily imported.
@sumpfralle
Copy link
Contributor Author

Could you please rebase this in light of the Sphinx changes being merged? That'll get rid of the failing CI and I can review in depth.

Done.

@Serene-Arc
Copy link
Contributor

Alright, this seems good to me. I just want to reiterate that beets developers may change these in future PRs; we can't really promise that the testing framework is stable or even tied to the version number, so be aware of that.

@Serene-Arc Serene-Arc merged commit 5c964ce into beetbox:master Jan 16, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants