Add unit tests for the run loop

[mdales]

As we push more functionality into the run loop, we need to find a way to write tests for it. The challenge is that it also deals with SDL matters which have side effects like creating windows, which is not unit test friendly.

Can we abstract the SDL functionality so we can mock it and have a run loop that deals just with the update mechanics?

[Enzujp]

Hi @mdales, what if I move the SDL functionality to a separate module like SDLBackend.ml and then define an interface to allow for mocking SDL for testing ?
And run would then be focused on the logic and not rendering

[Vanessa082]

Hi @mdales please I would love to take on this issue

[mdales]

Good luck @Vanessa082 - this could be a messy ticket, so take your time over it. Perhaps even start out with working out what you would want to test in the run loop, looking at why that is hard today, and then deciding from there how to restructure things to enable that.

[Vanessa082]

Thank you @mdales

[Vanessa082]

Hi @mdales, I am trying to work on this issue, but when I run the build command, I experience this

the configuration is in a dune file

(libraries tsdl giflib))

[mdales]

Ah yes, a dependancy was added. If you run dune pkg lock that should solve the issue - apologies.

[Vanessa082]

Hi @mdales, please. I am encountering this error from the base interface file. I have not altered, or touched the file or even remove the loop yet I have tried to open the key module,, but it did not work
Please will appreciate your help

[mdales]

I think it's to do with your changes to the dune file, which somehow have made it so that when you build the project it no longer finds the Key.mli/Key.ml files. I suspect it also ignores everything else also, just Key is the first thing in base.ml that it doesn't find. If you fix that it'll be whatever other things should be found. I got it to work by adding every other module to the dune file in the modules line, but given I don't know why that worked, I can't say that's the right thing to do :)

Whilst I can see the strategy you're going for in that PR, and in abstract it seems like a good idea (though I've not read it in detail) I think you need to be careful in doing that in that you're pushing beyond what either of us is familiar with in OCaml/dune :) That's not to say don't do it, but if we're going to play with those things, one of us needs to be sure we understand what's going on there, otherwise there might be other problems hidden in that strategy. Complexity that no-one understands isn't a good place to be in an open source project!

So either you need to go through the dune docs and understand why those changes you've made have broken things, or we need to find another way to tackle the problem. Does that make sense?

[Vanessa082]

I think it's to do with your changes to the dune file, which somehow have made it so that when you build the project it no longer finds the Key.mli/Key.ml files. I suspect it also ignores everything else also, just Key is the first thing in base.ml that it doesn't find. If you fix that it'll be whatever other things should be found. I got it to work by adding every other module to the dune file in the modules line, but given I don't know why that worked, I can't say that's the right thing to do :)

Whilst I can see the strategy you're going for in that PR, and in abstract it seems like a good idea (though I've not read it in detail) I think you need to be careful in doing that in that you're pushing beyond what either of us is familiar with in OCaml/dune :) That's not to say don't do it, but if we're going to play with those things, one of us needs to be sure we understand what's going on there, otherwise there might be other problems hidden in that strategy. Complexity that no-one understands isn't a good place to be in an open source project!

So either you need to go through the dune docs and understand why those changes you've made have broken things, or we need to find another way to tackle the problem. Does that make sense?

Hi @mdales
Thanks for the feedback and pointing out that the dune changes have caused the build to lose track of the key module. I made these changes because I got the same error about the module Base.
I get it that the abstraction approach I am attempting, moving the run loop into its own functor, has introduced unexpected module resolution issues and added complexity that may not be necessary at this point.
Based on your recommendation, I’m considering reverting to a more straightforward setup that is keeping the run loop in the base file and explicitly lists the modules in the dune file rather than creating additional abstraction layers. Will this approach be fine by you?

I’ll take a closer look at the dune documentation to understand why I have that problem

[mdales]

Both methods are fine by me, just so long as we understand what is going on. I do think I'd like to avoid us having to get to a place where in dune we have to update the dune file every time we add a new file, which I suspect is where we're going currently.

In general I have a preference for simpler solutions though, as they're easier to maintain :)

[Vanessa082]

Hi @mdales we will certainly go for the best alternative 😊 A simpler approach then

[Vanessa082]

Hi @mdales, sorry I have been slow with this ticket.