apprise icon indicating copy to clipboard operation
apprise copied to clipboard

Fix large throttling time issues in unit tests that use freezegun.freeze_time

Open ecourreges-orange opened this issue 1 year ago • 5 comments

Description:

Two fixes here:

  • Pushover service does not require throttling, so set throttling to 0
  • When using freeze_time for unit testing client code in "frozen time", this could lead to very large sleep times, which is a bummer in a unit test. Making the throttling elapsed time positive fixes that.

ecourreges-orange avatar Dec 26 '23 18:12 ecourreges-orange

The negative number indefinite sleep issue was fixed in Python v3.3. Apprise supports v3.6+

I'm not sure if this PR resolves a problem or not. Are you experiencing this? It is expressed that an IOError is thrown instead if this issue happens... But I've never seen it do that?

I have another PR coming that will fix the time during testing (by fixing it to 0s prior to every test; see #1020 (specifically this testing file places the disabled timeout before each test

caronc avatar Dec 27 '23 02:12 caronc

The negative number indefinite sleep issue was fixed in Python v3.3. Apprise supports v3.6+

The initial bug has nothing to do with negative sleep times, the elapsed is negative, so sleep(self.request_rate_per_sec - elapsed) is actually very positive (it was like 200k seconds for me).
Also I am using python 3.10 so not an issue here.
But indeed my fix introduces potential negative sleep times so it might not be ideal.

I'm not sure if this PR resolves a problem or not. Are you experiencing this? It is expressed that an IOError is thrown instead if this issue happens... But I've never seen it do that?

I have another PR coming that will fix the time during testing (by fixing it to 0s prior to every test; see #1020 (specifically this testing file places the disabled timeout before each test

This PR solves the problem of client unit tests, not unit tests of apprise, without adding the complexity for the user of having to know that he has to import conftest.py when testing his own code.

If you deem this an unnecessary fix, maybe I should only leave the commit that always disables throttling for pushover which doesn't require any throttling, this one is a nice addition that also solves my initial problem.

ecourreges-orange avatar Jan 08 '24 08:01 ecourreges-orange

I can't fully accept this solution as the Pushover API identifies they do throttle requests eventually. Setting the throttle limit to 0 may cause longer delays between messages once you start getting throttled from their end.

caronc avatar Jan 27 '24 19:01 caronc

The initial bug has nothing to do with negative sleep times, the elapsed is negative, so sleep(self.request_rate_per_sec - elapsed) is actually very positive (it was like 200k seconds for me).

this is definitely concerning... I've never seen/had this issue. Is your local machines time set correctly? Perhaps the issue is i'm using utc (or not?) and the bug simply involves the time we're comparing against is 'locally' incorrect (relative to the servers)? I'm not sure if that makes sense.

Basically hte server-time is being calculated against a different timezone value (thus hours off). I can't see 200K seconds though, there is something wrong there. 1 day (even if it were 24hrs off) is 86400 seconds.

caronc avatar Jan 27 '24 19:01 caronc

this is definitely concerning... I've never seen/had this issue. Is your local machines time set correctly? Perhaps the issue is i'm using utc (or not?) and the bug simply involves the time we're comparing against is 'locally' incorrect (relative to the servers)? I'm not sure if that makes sense.

Basically hte server-time is being calculated against a different timezone value (thus hours off). I can't see 200K seconds though, there is something wrong there. 1 day (even if it were 24hrs off) is 86400 seconds.

As I have mentioned, I have unit tests using freezegun.freeze_time with a fixed date pretty far in the past, thus possibility of datetime shifts forward and back.
The abs() was a proposition to avoid the 200k second sleep if other users of apprise were using the same lib as me and wasted time trying to figure it out.
For context I have a trading app that sends me notifications, and some unit tests involve trading hours which are shorter on holidays (like thanksgiving), hence I need to unit test on fixed dates+hours.

Would you like me to commit again without the pushover part, and with only the abs() part that avoids the possible very long sleep , so that the fix is safe?

Regards.

ecourreges-orange avatar Jan 29 '24 08:01 ecourreges-orange

I simply can't reproduce this, i'm closing it off as invalid. If you can set up an environment where it can be demonstrated, i'll gladly consider any PR you have.

caronc avatar May 11 '24 19:05 caronc