FooSoft / anki-connect

Anki plugin to expose a remote API for creating flash cards.
https://foosoft.net/projects/anki-connect/
Other
1.94k stars 219 forks source link

Fix getDeckStats test #322

Closed ZhengPeiRu21 closed 2 years ago

ZhengPeiRu21 commented 2 years ago

Sorry that I did not see this earlier! When I first implemented getDeckStats, it supported only one deck per call. When updated to support multiple decks I did not properly update the unit test. This should resolve the CI build error due to the test failing after https://github.com/FooSoft/anki-connect/pull/318. I apologize for any trouble!

ZhengPeiRu21 commented 2 years ago

It seems this unit test is still having a trouble:

 =================================== FAILURES ===================================
______________________________ test_getDeckStats _______________________________
session_with_profile_loaded = <pytest_anki._session.AnkiSession object at 0x7f7c21504250>

    def test_getDeckStats(session_with_profile_loaded):
>       result = ac.getDeckStats(decks=["Default"])

/home/runner/work/anki-connect/anki-connect/tests/test_decks.py:73: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/runner/work/anki-connect/anki-connect/plugin/__init__.py:645: in getDeckStats
    responseDict[deckId] = self.deckStatsToJson(deckNode)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <plugin.AnkiConnect object at 0x7f7c37789220>
due_tree = deck_id: 1
name: "Default"
level: 1
collapsed: true

    def deckStatsToJson(self, due_tree):
        return {'deck_id': due_tree.deck_id,
                'name': due_tree.name,
                'new_count': due_tree.new_count,
                'learn_count': due_tree.learn_count,
                'review_count': due_tree.review_count,
>               'total_in_deck': due_tree.total_in_deck}
E       AttributeError: total_in_deck

total_in_deck is a valid attribute of Anki DeckTreeNode, so I am not sure of why this error is appearing! I may need some guidance to solve this test. This is strange because the call itself works well in real usage, so only the unit test is having an issue.

oakkitten commented 2 years ago

It seems that the test only fails on Anki 2.1.46 and below; does total_in_deck exist on Anki 2.1.46?

Also, you can run tests yourself. See the comment in tox.ini for instructions

oakkitten commented 2 years ago

Oh, and this change has way too many lines changed. Perhaps an issue with line endings?

ZhengPeiRu21 commented 2 years ago

It does seem to being a line ending problem. I will try to fix it.

With regarding the Anki lower version issue, is there a preferred way of handle behavior that is different between Anki versions? Should I remove total_in_deck from the stats completely, or should there be a separate codepath that adds it if supported?

oakkitten commented 2 years ago

I'm no authority here, but if you asked me, there are four ways, in order of least preference:

  1. Remove it;
  2. Don't include it in the results if running on Anki that doesn't support it, or return some useless value, and add a note in the comment;
  3. If it is computable, compute it where necessary and include in the results. We've got anki_version that you can check against;
  4. Figure out if maybe it's worth bumping out the minimum supported Anki version.

Anki mixes “stable” versions (e.g. 2.1.49) and not-quite-stable (e.g. 2.1.50). I'm not sure if 2.1.45 and 2.1.46 are stable, or in other words, that many users use these versions. If we can figure which versions are not stable, we can drop support of them. That would be the best solution I guess? (Users who do use versions like 2.1.45 will still be able to use Anki-Connect, just without the new updates.)

ZhengPeiRu21 commented 2 years ago

I have added a version check and only add the total_in_deck value in supported versions. It seems the line endings are still not correct; I will fix this next. Thank you for your helping!

ZhengPeiRu21 commented 2 years ago

I have fixed the line endings. Thank you for the patience!

oakkitten commented 2 years ago

Nice! Perhaps also update the readme to say that total_in_deck will only be returned on api 2.1.47+.

It would also be nice to test some numbers maybe? The tests run in isolation so you can count on them being zeros. Or you can test using the setup fixture, it has a few cards.

(P.S. This is a matter of personal taste; I prefer to say if anki_version >= (2, 1, 47) not if anki_version > (2, 1, 46), I just it's nice to refer to the “the first version that supports“ than to “the last version that doesn't support”.)