kiorky / croniter

MIT License
410 stars 40 forks source link

Unused equality comparisons in `test_speed.py` #139

Closed qTipTip closed 4 days ago

qTipTip commented 4 days ago

Hello! 👋

I would like some clarifying input on the test_speed.py file.

Specifically lines like these:

https://github.com/kiorky/croniter/blob/43a3f6ef22ae649628b284c1271bda213ee1b3d7/src/croniter/tests/test_speed.py#L27-L31

Is the intention to verify that base.year equals n1.year, and so on? If so, I believe there is an assert missing here unless there is some other mechanism by which comparison is checked here.

If these tests indeed do not verify the intended behavior, I'd be happy to take a stab at fixing them, but I wanted some input from you first.

kiorky commented 4 days ago

the purpose of this test is to be repeated, see the __main__ section, this tests a large part of most resource consuming croniter code and will be as slow as hell if there is a regression.

kiorky commented 4 days ago

(this is a test which is not really integrated in the test suite, as you can see, historically it has to be run manually.)

I ll see in a next release to modernize and integrate it in the regular test suite.

kiorky commented 4 days ago

I reworked this test which will be part of next release.

You can see the pending changes here : https://github.com/kiorky/croniter/blob/wip/src/croniter/tests/test_croniter_speed.py

I hope things are more clear now :-)

qTipTip commented 3 days ago

Aha, I see! Thanks for clarifying, @kiorky! 🥇 Also, thanks for maintaining this package!