Fix sonnet autodetect test failure
ClosedPublic

Authored by dfaure on Mar 29 2020, 1:50 PM.

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.
waqar created this revision.Mar 29 2020, 1:50 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 29 2020, 1:50 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
waqar requested review of this revision.Mar 29 2020, 1:50 PM
dfaure added a subscriber: dfaure.Mar 29 2020, 2:41 PM

I'm no expert in this stuff, but here's my feedback

  1. on my own system (!= CI), all three tests were failing before, due to finding en_AU-variant_1, en_AU-variant_1 and "".
  2. 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?

apol retitled this revision from "Fix sonnet autodetect test failure" to Fix sonnet autodetect test failure.Mar 29 2020, 4:30 PM
waqar added inline comments.Mar 29 2020, 7:30 PM
autotests/test_autodetect.cpp
51

Yes, behind the scenes it is saved in the Settings.

waqar added a comment.EditedMar 29 2020, 7:41 PM

I'm no expert in this stuff, but here's my feedback

  1. on my own system (!= CI), all three tests were failing before, due to finding en_AU-variant_1, en_AU-variant_1 and "".
  2. 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-*?

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?

dfaure requested changes to this revision.Apr 8 2020, 4:57 PM
This revision now requires changes to proceed.Apr 8 2020, 4:57 PM
dfaure commandeered this revision.Apr 11 2020, 11:52 AM
dfaure edited reviewers, added: waqar; removed: dfaure.
This revision was not accepted when it landed; it landed in state Needs Revision.Apr 11 2020, 11:52 AM
Closed by commit R246:78450298149e: Fix sonnet autodetect test failure (authored by waqar, committed by dfaure). · Explain Why
This revision was automatically updated to reflect the committed changes.

@waqar ping? What do you think about my suggestion? If you agree, can you update the patch?

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.