Details
- Reviewers
waqar - Commits
- R246:78450298149e: Fix sonnet autodetect test failure
Diff Detail
- Repository
- R246 Sonnet
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
I'm no expert in this stuff, but here's my feedback
- on my own system (!= CI), all three tests were failing before, due to finding en_AU-variant_1, en_AU-variant_1 and "".
- now the test passes locally, but because all tests are skipped
SKIP : SonnetAutoDetectTest::autodetect(English) en_US-large not available
SKIP : SonnetAutoDetectTest::autodetect(German) de_DE_frami not available
SKIP : SonnetAutoDetectTest::autodetect(Malayam) ml_IN not available
What's en_US-large? Can we at least check that we get en_US-*?
Thanks.
autotests/test_autodetect.cpp | ||
---|---|---|
51 | This will have an effect on *other* instances of the Speller class? |
autotests/test_autodetect.cpp | ||
---|---|---|
51 | Yes, behind the scenes it is saved in the Settings. |
en_US-large is the US English dictionary name, the most common one available in most distros according to my findings. Looking for en_US-*/en_US* would still make the test fail. I have both ASpell and Hunspell installed on my system, so i have quite a large list of English dictionaries available. Every time I run the test, it autodetects a different variant of 'English' i.e., en_AU-variant_1/ en_US / en_GB. So, to make the test pass, I chopped the string and retrieved the language code only and used that for comparison. For e.g, for en_US -> en is the language code.
The tests are currently failing because it was unable to find any en_US/de_DE_frami in the dictionaries because of:
if (!speller.availableLanguages().contains(correct_lang)) { const QString msg = correct_lang + QStringLiteral(" not available"); QSKIP(msg.toLocal8Bit().constData()); }
but perhaps we can get around it using regex to look for language code only instead of the full dictionary code.
Well if the test is tolerant about correctLangCode, it might as well not require a specific variant upfront.
What about replacing en_US-large with en_US and de_DE_frami with de_DE in SonnetAutoDetectTest::autodetect_data?
This makes the first test pass for me (I don't have de_DE or ml_IN installed).
@waqar ping? What do you think about my suggestion? If you agree, can you update the patch?
I pushed it with my changes included. The test is fixed: https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/sonnet/job/kf5-qt5%20SUSEQt5.12/
Sorry I have been really busy at work, and overlooked the last email. I did plan to work on this today though 😆
and you already committed the changes.
It looks great. I have a couple of ideas to improve this though, So i will create another patch.
There are other sonnet bugs that I want to work on in the meantime. And if you have something particular in mind, you can point me there.