test: ✅ mocking of basic app functionality

[joelostblom]

This is a first implementation of the tests we can use to check that the CLI is behaving correctly. I think this covers most of what we currently have in main, at least we reach 💯 % coverage!

Merging this before some of the existing PRs e.g. #140 and #152 will make it easier to add new tests specifically for the functionality added there instead of convoluting it with testing the general functionality of the app which is tested here instead.

I tried to follow the cyclopts test docs as closely as possible, but let me know if you think something is missing. I also experimented with using the Copilot CLI for the first time to help me get started and understanding how to write the tests. It was quite useful, particularly to get started, but I did have to delete about half of its suggestions because there was a lot of redundancy. Thought I would mention it in case someone catches something odd in here so I can blame it and not me =p

Needs a thorough review.

Checklist

• Ran just run-all

[lwjohnst86]

Nice! Some I don't understand the code but understand the concept/intent. Some small questions/comments.

[martonvago]

Very nice, I look forward to using this 😁

[martonvago]

Just one question:
How do we imagine this will impact testing internal functions like _resolve_uri or _build_sections?
We can already test these in isolation but choose not to because they are internal.
With this PR, we will be able to call a public function and spy on the internal functions to test them in that context. Is that much different / better? Or will we still not test internal functions?

[joelostblom]

@martonvago Great question! And interesting find with spy, I didn't know of it. I agree with you that using it seems similar to just testing the internal functions directly.

What do you think about the asserts used in test_build_with_mocked_internals where we check that the correct object is passed to the different functions inside build? If the correct object is passed to e.g. _read_properties it means that _resolve_uri must be doing the right thing internally. Is it sufficient to test that the right object is passed around like this and not care about what is happening internally as long as the return value looks the way we expect?

[lwjohnst86]

Add the helper function and I will approve after ☺️

[martonvago]

@joelostblom What test_build_with_mocked_internals is testing is just the wiring together of the internal functions in build. The mocks are just dummies, the functions they are mocking are not executed at all. mock_resolve_uri returns fake_path because the mock is configured to do so; _resolve_uri is not actually executed in the test.

So this test makes sense if we want to test that build calls the correct functions in the correct order, but it doesn't test the logic inside the internal functions.

[joelostblom]

Ah yes, you're right, I set the return value from mock_resolve_uri manually, duh 🤦 🧠 💨

Hmmm... what do you think of the approach we mentioned before regarding testing that the eventual output files are written with a correct structure in the correct location? That's of course a more indirect way and doesn't test each individual internal function so it might be hard to tell what is failing inside. Maybe we can try that and see if it clear enough from the stack trace what is going awry, and otherwise move to explicitly test internals if there is something that is so complex that it is hard to tell what went wrong if we don't explicitly test the internal function directly?

[martonvago]

Yeah, I'm happy with that!
We'll just have to make sure we have test configs/styles that have examples of all edge cases.
And we'll have to decide if we want to keep a collection of expected output files and test actual output files against those, or if we want to be more relaxed about the exact contents of actual output files. There are some things (e.g., concatenating content items in the same section) that can only be tested by inspecting the contents of the output files.

[joelostblom]

Sounds good! I split our discussion out into its own issue so that it doesn't get lost in this PR #156

[lwjohnst86]

Very nice! Only some small things with the tool script.

[lwjohnst86]

Much better, thankkksss ❤️