Adding tools for managing test fixtures

[dgkf]

Hello! Thanks for this great resource!

I recently noticed that the id field was missing from an entity that I want to use (#198), and then saw that there's an old issue about revising the testing strategy to not rely on static data (#110), which is probably why it's so easy to miss these changes.

I hope you don't mind that I went ahead and added some tools to fix this. I'm not entirely sure what the best practices are for things like this, so feel free to treat this like a starting point. If you have thoughts on a better way to approach it, I'm happy to see this evolve into something that is more idiomatic.

Just to outline the approach:

• I added a new binary build target, which just builds a small CLI that developers can use to refresh specific files:

  # update all the test files itemized in `src/fixtures.rs`
  cargo run --features fixtures update
  
  # update specific files by glob pattern matching
  cargo run --features fixtures update --path **/*Frozen*

  I'm torn on this approach. It's nice to have an executable bundled with the package to support things like this, but it makes the dependencies and features a bit messy since it fuses developer needs with user-facing needs.

• I introduced an initial commit just to re-format the original json files to make them easier to diff. Using long-form json will be easier to diff when checking for changes in the API results. Json file changes in the second commit show the diff in a more interpret table format.

Will annotate other decisions in-line in the PR

[dgkf]

I used the internal request client directly to issue manually-constructed API requests. I went with this route instead of using the package itself because I wanted to store the test data as close as possible to how it gets received from the API.

[dgkf]

Since I'm using the raw request client, rate limiting is done in a rather simple way.

[dgkf]

Fixtures are defined like this, just to get something actionable off the ground. There are probably more thoughtful ways of organizing this data, but I wanted to be minimally disruptive to other tests for now.

[dgkf]

serde is used to parse the json and output pretty-printed long-form json for easier diffs. One consequence of this is that non-ascii characters aren't always 1-to-1 with the original body. serde_json_fmt is used to encode them as similarly as possible, but some characters get encoded that aren't originally encoded (so far I've seen - and ~ get escaped by serde_json_fmt, where they are left as-is in the original body).

[dgkf]

As far as I'm aware, this is the only part of a query that I couldn't reproduce. I tried enabling every type of relation exposed by the API but could not recover the instrumentalist information.

[RustyNova016]

I haven't reviewed the code yet so this is just process talk for now.

---

While this is a pretty smart idea, I'm a bit torn on it's usefulness. Usually, api bindings requires three types of tests:

• I can ping the api, and the api pongs back without issues
• I can read the data from the api, and turn it back into the original data (Syntactically equivalent. Doesn't have to be the exact same formatting)
• The api gives me correct data for what I asked

The second point is a solved issue. I got some tests that serialize and deserialize the data and check it against a serde_json::Value of the original response. Not all the tests are migrated to it as the two other points blocks it

The first point is easy in principle, but a bit of a pain. Testing all the possible includes and endpoints requires a lot of queries, and with a limit of 1request per second, it gets slow

The final point is the hardest. You need to check that you get what you expect from the api (Like the includes, or having the result you searched for), but without being too specific about it so that there's the least opportunity for the data to change.

---

Your PR would fix 2), but lacks in the other points. You only test if the endpoints are correct by manually triggering a regeneration of the fixtures, and changing fixtures could remove the point of a test (For example, you can't really test to see if there's a recording relation as if it gets removed from MB, the fixture and the tested data will be the same still)

Like I said in #110, I think a fully dynamic approach would work better. From what I already have as a new test framework, I can test both that the schemas are valid, and that the endpoints works. But like you, I just can't guaranty that the test pass because the schema is valid, or that the thing I'm testing doesn't exists anymore on MB

I created an issue on cargo mutants to hopefully help with that by just removing each fields and seeing if it breaks. Barbaric, but it would works!
sourcefrog/cargo-mutants#605

If you have any other ideas, please feel free to share.

[dgkf]

I'm a bit torn on it's usefulness.

Yeah, I share that sentiment. I think a proper testing strategy would still need to go beyond this.

This is a bit of machinery to keep the tests for goal (2) up-to-date. I put it together to try to discover any other data model mismatches.

I'd say the value here is having parallel raw ureq queries and musicbrainz_rs queries that can be checked for equivalence. If nothing else, hopefully that helps make some progress toward goal (3).

A better solution might run a scheduled CI job to run specific round trip tests, querying (and caching?) api responses to catch data model mismatches, but I didn't want to contribute something that opinionated. This would avoid slowing down more frequent CI with a ton of rate-limited queries while still catching API response changes relatively quickly.

As it is now, it's really just a basic script - no worries if you decide it's not in the style you want. If that's the case, feel free to close and take this as food for thought.

[RustyNova016]

Welp as an update, I finally made the testing utils in api_bindium. Allows for testing proper urls, deserialising, and roundtrip. Example here

It doesn't fix the issue that I will have to test against some static data, but I think I can reduce it, and reduce the numbers of individual include checks to none and all.