FM-17 / poglink

A bot that integrates the ARK Web API with Discord.
MIT License
12 stars 4 forks source link

Integration tests didn't catch non-existent `rates_diff` object #44

Closed FM-17 closed 2 years ago

FM-17 commented 2 years ago
2021-12-19 23:38:22,967:INFO:poglink.cogs.rates: Cog Ready: Rates
Ignoring exception in on_ready
Traceback (most recent call last):
  File "/home/fm/.pyenv/versions/3.7.0/envs/venv6/lib/python3.7/site-packages/discord/client.py", line 343, in _run_event
    await coro(*args, **kwargs)
  File "/home/fm/.pyenv/versions/3.7.0/envs/venv6/lib/python3.7/site-packages/poglink/cogs/rates.py", line 174, in on_ready
    if rates_diff.items:
AttributeError: 'coroutine' object has no attribute 'items'
/home/fm/.pyenv/versions/3.7.0/envs/venv6/lib/python3.7/site-packages/discord/client.py:350: RuntimeWarning: coroutine 'Rates.compare_posted_rates' was never awaited
  pass

This error was thrown due to the following line not being awaited in rates.py

rates_diff = self.compare_posted_rates(url, output_path)

This issue was resolved. However, the integration tests passed without issue.

Need to investigate why these passed.

travipross commented 2 years ago

I've taken a moment to review closer and I think I realized why this happened, and I now believe this was just an issue of a mistake that lied outside of reasonable test coverage. My reasoning:

Technically the integration test for the cog will never truly be end-to-end as long as it's not calling on_ready() directly, but to do that feels non-trivial/unreasonable because of the while True loop that is required in that method.

I propose we close this issue and just chalk the problem up to being a very narrow edge case that isn't covered by tests and happened to have a syntactic error. The alternative would be to look into methods of testing while True loops which contain async code, but then you'd have to be concerned with the timing of the tests because you'd have to ensure a predictable number of iterations took place asynchronously before making your assertions (because the function isn't stateless, and the number of iterations will change the outcome). That just doesn't sound fun lol

FM-17 commented 2 years ago

Agreed.