Add support for all time based schedulers in AppDaemon

[lifeisafractal]

Currently, only run_in is support by the test framework. It would be great for testing if all scheduler calls were supported as well as dispatching these callbacks. Additional, a more expressive API for moving through time in tests would help cover more test cases I'm interned in when TDD'ing my tests.

I'm currently working on a branch with some ideas of how to approach this if you want ot have a look: https://github.com/FlorianKempenich/Appdaemon-Test-Framework/compare/master...lifeisafractal:expand_time_travel

I'd like to discuss design decisions here before I get too far down the wrong path, but I'm also happy puttering along on this (hey, it's fun!).

General design thoughts:

• Make the time_travel fixture the central place for handling all time and callback scheduling.
• Rather than mocking each call (run_hourly, run_at, etc.), mock lower level AD APIs so we can reuse the appapi implementations and logic. (i.e. appapi.insert_schedule)
  • Pros:
  • Less code to write
  • Behavior during test is exactly as it will be in the wild
  • Cons:
  • Depends on the internal API structure of AppDaemon. The upcoming 4.0beta already would break this, but is fixable
• Proposed breaking change: remove given_that.time_is() and replace with time_travel.reset_time(). If the time_travel fixture takes over the whole notion of time in the system, using the given_that fixture to set time could produce inconstant tests or require much more coupling between given_that and time_travel which are currently unaware of each other. I'd love to see a solution that could keep this API cleanly, but right now I'm leaning to the best thing being to bite the bullet and make the breaking change.

I'll keep putting along on my branch, but I'd love to talk about why this is needed and how to implement it as I'm going along on it.

Also, the intent would be to get this up for PR and merged, but then follow on with an extension that support sun related callbacks and calls.

[HelloThisIsFlo]

First of all, as always, I love your enthusiasm 😃

I think expanding the time capabilities is a great idea! One that I wanted to tackle a while ago, but didn't end up having time to implement. And now, I don't have the use-case anymore. That would be nice but not enough for me to spend too much time on it. So when I saw, you were taking a shot at it, I was more than pleasantly surprised 😊

Make the time_travel fixture the central place for handling all time and callback scheduling.

👍

Rather than mocking each call (run_hourly, run_at, etc.), mock lower level AD APIs so we can reuse the appapi implementations and logic. (i.e. appapi.insert_schedule)

That was one of my original ideas, but after looking a bit into it I concluded it wasn't worth the hassle for my use-case at the time. If you find a way to build it, and if it's reliable enough, then Hell yeah, go for it !!

Proposed breaking change: remove given_that.time_is() and replace with time_travel.reset_time().

I see the rationale, however I'm not sure keeping the current API would necessarily mean cutting corners on code quality. Here is how I see it:

• For sure, 100% agree having responsibilites spread accross multiple classes is something to watch out for. However ...
• ... I really like the current structure of having given_that taking care of setting the stage. given_that is the central place for all setup.

So I would be into keeping given_that.time_is(...).

Now, would that necessarily mean breaking encapsulation in nasty ways? Not necessarily:

• We could have given_that.time_is(...) call time_travel.reset_time(...), on top of what it's already doing. (and make reset_time(...) internal use only).
• But a better alternative maybe could be:
  • Extract all the mock state behavior to a separate class, used interally only, and inject it as dependency of both given_that and time_travel.
  • We could call it for instance MockStates (or whatever feels right), and it'll hold not only the current time, but also all the other mocked states.
  • Then:
  • given_that would set all the mocked states in MockState, as well as the mock time (it's some sort of state after all)
  • time_travel would read from MockStates whenever it needs to know when is the current time.
  • We could have a dirty flag in MockStates set by time_travel whenever a callback is registered to prevent the time to be set again in the middle of a test.

With one of these alternative we would have:

• Clean separation of concerns 😃
• The amazing new behavior
• No breaking changes (quite important since it's not just the 2 of us using the test "framework", but I also agree it shouldn't be sacred 🙂)

Anyway, let me know what you think 😉 Also, important: I want you to have fun when working on this. So if you disagree with whatever I'm saying, it's totally cool, let's have a discussion, that's what this place is for 🙂 All I'm saying is: Never go with a decision you don't agree with, let's talk until we find a compromise we're both happy with (and I guarantee, if we're patient, we always will ;) )

[lifeisafractal]

I think you're on to something there with that new code structuring! I think factoring out internal helper fixtures to hold state is a great answer and can keep the current test API the same. I also agree that given_that makes most sense for setting stuff up. I wanted to get there, but couldn't quite figure it out on my own.

I'll keep working on my branch and post a PR when I have something work talking about.

Also, thanks for the last comment. I'm for sure having fun and you've been a please to work with :).

[lifeisafractal]

I scrapped what I had and took another stab at this based on the discussion above and overall I'm really happy with how it's coming out. I've hit one real problem I can't get through.

With full simulation all scheduler types, you can't just jump around in time without possibly misrepresenting the true state of things (both scheduled events and states). This means, you need to set the start time prior to scheduling any callbacks, which practically means before you .initialize() the automation. The current pattern of calling initialize() in the test fixture setup makes this pretty clunky. If you want to set the start time to different things for different tests this makes it pretty complicated.

Do you have any thoughts? I'll keep playing around with it, but I'm not sure how to do this with the least breaking impact.

[lifeisafractal]

FYI, here is the new branch I'm working on if you want to take a look at it: https://github.com/FlorianKempenich/Appdaemon-Test-Framework/compare/master...lifeisafractal:expand_time_travel_take2

[HelloThisIsFlo]

(quick update on my status)

I'm currently in the process of moving cities, countries even, without a job lined up. It's something I'm used to doing, so all good, but it does mean that my availabilities are really sparse for the next few weeks.

I read all your comments but didn't yet find the time to check out your branch to play with your implementation. I'll take a deeper look when I find some downtime on my side, I just don't know if it's going to be in 2 days or in 2 months (both being extremes ;) ) Anyway, just wanted to let you know that even though I don't answer, I do read everything and follow your progress, from a distance 😊

You'll hear more from me .... when you'll hear more from me 😊

[lifeisafractal]

thanks for the update. I'll keep working on that branch. Good luck with the move!

[lifeisafractal]

Quick follow up: I've made some great progress that is moving in on some reviewable code. I need to up date the docs (which will help digesting the PR also), and need to re-vamp the existing example integration tests to fit some of the new patterns better. Once I get those done, I'll open a PR so we can begin discussion against the code. This is going to be a bigger one so I'm ready for some serious iteration to get to a point where we're all happy.

[HelloThisIsFlo]

I'm closing this, let's continue the discussion on the PR ;)