WordPress / openverse

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

Add serializer unit tests #724

Open AetherUnbound opened 2 years ago

AetherUnbound commented 2 years ago

Problem

We do not currently have unit tests for any of the API serializers (api/catalog/api/serializers/*).

Description

We should add thorough unit tests to these serializers, focusing on the base functions in the media_serializers.py file.

Additional context

This came up as part of WordPress/openverse-api#526.

Implementation

sarayourfriend commented 2 years ago

A note for whomever picks this up: please continue to extend and use the factories for models. Currently there's only factories for the Audio models and the base media factory needs to be extended further to include more of the fields, especially if we're going to unit test all the serializer fields :slightly_smiling_face:

foroveralls commented 2 years ago

Hey would be interested in working on this. I'm however new to this project and not familiar with factories, or the standard procedure for unit testing here, could someone provide some pointers to reference files?

sarayourfriend commented 2 years ago

Hi @foroveralls! Thanks for offering to work on this.

Most of our test coverage is currently in integration tests, so we only have a few unit test examples, but I'll share them here to make it easier to find.

The best factory example is the AudioFactory and AudioAddOnFactory, found in this module: https://github.com/WordPress/openverse-api/blob/c09fd7e16a8eb104c311e8d4f0da08238570067c/api/test/factory/models/audio.py

From there you can inspect the base MediaFactory which will need some expanding in addition to adding a new ImageFactory based on it (similar to AudioFactory).

We use the popular factoryboy library (docs link here) to build Django model factories and other data factories. We've extended this with additional faker providers in this module: https://github.com/WordPress/openverse-api/blob/188b6a0c299a4b79e205c997378e230bf1f96d14/api/test/factory/faker.py

The unit tests themselves are currently limited to a management command and tests for the waveform generation logic on the Audio model. Those are linked here:

https://github.com/WordPress/openverse-api/blob/c09fd7e16a8eb104c311e8d4f0da08238570067c/api/test/unit/management/commands/generatewaveforms_test.py

https://github.com/WordPress/openverse-api/blob/c09fd7e16a8eb104c311e8d4f0da08238570067c/api/test/unit/models/audio_test.py

Generally we follow a pretty standard Django unit testing model, but with pytest instead of the Django unit test classes.

If you need any clarification about the above or additional help, please feel free to ping. And thanks in advance for your contribution! If this all sounds good to you, please confirm and we'll assign the issue to you :grin:

foroveralls commented 2 years ago

Thanks for the comprehensive response, looks good I'll have a read and I'm sure I'll figure it out, please assign it to me.

No problem!

sarayourfriend commented 2 years ago

Awesome! BTW, please feel free to open smaller, atomic PRs rather than a single large PR adding unit tests to everything. It'll make getting reviews from maintainers a bit easier and should move things along faster in the end :slightly_smiling_face: And don't hesitate to open draft PRs, we love those around here!

foroveralls commented 2 years ago

For some reason my touchpad spasmed and clicked unassigned me, if you could re-assign me that would be excellent :)

akhilsrivatsa commented 1 year ago

I can pick this up.