caronc / apprise

Apprise - Push Notifications that work with just about every platform!
https://hub.docker.com/r/caronc/apprise
BSD 2-Clause "Simplified" License
10.94k stars 385 forks source link

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

Closed ecourreges-orange closed 1 month ago

ecourreges-orange commented 6 months ago

Description:

Two fixes here:

caronc commented 6 months ago

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

ecourreges-orange commented 6 months ago

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.

caronc commented 5 months ago

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 commented 5 months ago

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.

ecourreges-orange commented 5 months ago

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.

caronc commented 1 month ago

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.