WordPress / openverse

Openverse is a search engine for openly-licensed media. This monorepo includes all application code.
https://openverse.org
MIT License
223 stars 179 forks source link

LicenseInfo for specific licenses in catalog tests should be extracted #1978

Open obulat opened 1 year ago

obulat commented 1 year ago

Problem

Currently, several tests declare various license_info constants. This leads to duplication.

Description

We should extract the license constants used in tests to a separate module, catalog/tests/testing_utilities/licenses.py, and import them from there.

Alternatives

We could also define the constants in the common/licenses, but there are a lot of possible licenses, so I think defining the constants that are specifically used in tests would be simpler.

Additional context

Extracted from a PR comment: https://github.com/WordPress/openverse/pull/1898#discussion_r1181719389

krysal commented 1 year ago

I believe the dags/common/licenses/constants.py file should be a good place for those constants.

stacimc commented 1 year ago

If these are only used in tests, and especially if we're only defining a subset of licenses as constants, it might be preferable to keep them in the catalog/tests/ directory.

obulat commented 1 year ago

If these are only used in tests, and especially if we're only defining a subset of licenses as constants, it might be preferable to keep them in the catalog/tests/ directory.

Yes, these are used only in tests, and there's just a subset of the most popular licenses. That is why I didn't think dags/common/licenses/constants.py was the best place for it. Do you think something like catalog/tests/licenses.py would be a good place, @stacimc? Or should we create a folder for them, like catalog/tests/constants/licenses.py?

AetherUnbound commented 1 year ago

When we were working on https://github.com/WordPress/openverse-catalog/pull/1059, one open question was where to put the utility function. I think if/as we start to have more of these functions & utilities, maybe it makes sense to have an explicit place in the testing folder to put testing utilities. Potentially catalog/tests/testing_utilities (can't be test_utilities otherwise that'd look like a test to pytest potentially), what do you think? Then we could have this under catalog/tests/testing_utilities/licenses.py.

stacimc commented 1 year ago

Agreed! There's an existing catalog/tests/utilities directory for literally testing things that are utilities, which could maybe cause confusion. I like the suggestion for catalog/tests/testing_utilities/licenses.py :)

sarayourfriend commented 1 year ago

I've updated the issue description to reflect @stacimc's suggestion and the various positive emoji reactions on it and removed the ticket work required. I originally added "help wanted" as it seems straightforward enough for a community member to implement, but more details would be needed for that. Specifically: what information are you referring to in the issue description @obulat? Can you add links? If you do, please add the "help wanted" label afterwards. Optionally, this can be left ambiguous.