Skip to content
This repository has been archived by the owner on Dec 31, 2023. It is now read-only.

Fix getDeckStats test #322

Merged
merged 3 commits into from
May 29, 2022
Merged

Conversation

ZhengPeiRu21
Copy link
Contributor

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 #318. I apologize for any trouble!

@ZhengPeiRu21
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@ZhengPeiRu21
Copy link
Contributor Author

ZhengPeiRu21 commented May 19, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@oakkitten
Copy link
Contributor

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”.)

@FooSoft FooSoft merged commit 1d5b87b into FooSoft:master May 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants