Additional AssetPath unit tests.

[viridia]

Objective

Additional unit test for AssetPath.

[alice-i-cecile]

These are good tests! Kind of a nit, but I'd split these up into one assert per test. There's no setup and it makes debugging failures way easier.

[viridia]

These are good tests! Kind of a nit, but I'd split these up into one assert per test. There's no setup and it makes debugging failures way easier.

Well, I'm just following the pattern for the tests already there - admittedly I wrote most of the tests in this file, but the ones I didn't write also have multiple asserts per tests.

That being said, I understand where you are coming from. I like to write tests that are linear and which "tell a story", meaning that if you read the tests in order it's like reading documentation. The test failure report will tell you the line number, so you'll know which assert it was.

[alice-i-cecile]

Yeah, I can get behind that. I wouldn't have hassled you about it if you were a new contributor / junior engineer, but I figured it's a good opportunity for a conversation :)

Definitely consistent with the existing tests. My main gripe with it is when you get multiple panics in the same test, and they could be evaluated separately, it slows down your debugging by a cycle or two. No big deal though, this LGTM :)

[viridia]

Test granularity is definitely more art than science!

[viridia]

@alice-i-cecile The more interesting question, I think, is "what else in this file should be tested?".

I generally don't like tests for simple accessors, they are just clutter and don't add value. But anything that has an algorithm, or a non-obvious assumption, is fair game.

For example, it may seem silly to write a test that tests the value of a constant - unless that constant needs to have special properties, like being a prime number. The test guards against future programmers accidentally making the number non-prime.

I'm not sure that there's much value in testing the "Display" impl, since that's mostly used for debugging.