1204540 - Spell checker tests need clean-up. Some leave content preferences set and make the next test fail.

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Jorg K (CEST = GMT+2)]

While working on bug 697981 I notices that my test that works locally as a single test fails in the test suite when run with many tests.

Looking for my debug "writing content preferences" in

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e5af6b2c0ea8/try-linux64/try_ubuntu64_vm_test-mochitest-other-bm118-tests1-linux64-build849.txt.gz

I can see that there are 11 cases in the following tests:

1) test_async_UpdateCurrentDictionary.html
***** Writing content preferences for |testing-XX|

2) test_bug1200533.html
***** Writing content preferences for |en-US|

3) test_bug338427.html
***** Writing content preferences for |en-US|

4) test_bug678842.html
***** Writing content preferences for |en-GB|
***** Writing content preferences for |en-US|

5) test_bug717433.html
***** Writing content preferences for |en-GB|
***** Writing content preferences for |en-US|

6) test_add_remove_dictionaries.xul
***** Writing content preferences for |base-utf|
***** Writing content preferences for |maputf|
***** Writing content preferences for |base-utf|
***** Writing content preferences for |en-US|

We should clean this up, since new tests will fail in weird and wonderful ways depending on what previous tests have left behind.

Comment 1

[Jorg K (CEST = GMT+2)]

There could be some side effects from bug 1204147. We need to count again after that bug landed.

Comment 2

[Jorg K (CEST = GMT+2)]

I ran the test again with the patch for 1204147 included:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=122f1e4fdbc6

That greatly reduced the list of content preferences written:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-122f1e4fdbc6/try-linux64/try_ubuntu64_vm_test-mochitest-other-bm53-tests1-linux64-build597.txt.gz

In total we now have:
1) editor/composer/test/test_async_UpdateCurrentDictionary.html
***** Writing content preferences for |testing-XX|
Analysis: line 56: SetCurrentDictionary("testing-XX");

2) editor/composer/test/test_bug338427.html
***** Writing content preferences for |en-US|
Analysis: line 37: spellchecker.spellChecker.SetCurrentDictionary(lang);

3) editor/composer/test/test_bug678842.html
***** Writing content preferences for |en-GB|
Analysis: line 68: spellchecker.SetCurrentDictionary('en-US');

4) editor/composer/test/test_bug717433.html 
***** Writing content preferences for |en-US|
Analysis: line 73 (or 76): spellchecker.SetCurrentDictionary("en-US");

5) extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul
***** Writing content preferences for |base-utf|
***** Writing content preferences for |maputf|
Analysis: line 75: setCurrentDictionary(editor, "base-utf");
          line 83: setCurrentDictionary(editor, "maputf");

The differences to comment #0 are caused by the fix in bug 1204147. 
In other words: The fix in bug 1204147 killed five cases where content preferences were written erroneously. Fantastic, so all the content preferences written here are expected.

Now, the problem remains:
How do we fix these five tests to clean up after themselves?

Are there a few lines of script I can use, to remove the content preferences when the test finishes?

I've looked at "removeGlobal" but couldn't make much sense of it:
http://mxr.mozilla.org/mozilla-central/ident?i=removeGlobal

Help!

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I'm not really familiar with APIs dealing with content preferences, sorry. 
So I would as well just need to go through the existing tests and other code to see how to do that.

Comment 4

[Jorg K (CEST = GMT+2)]

Who can I ask? I found toolkit/components/contentprefs/tests/unit_cps2/test_remove.js, and being not much of a JS programmer, can't make too much sense of that. Looks like the interface is also asynchronous, which makes harder still.

Let's try Ehsan. He's always good for a quick tip. Executive summary and question ;-)

In a test that sets content preferences, how can I remove them once the test is done, so the next test along doesn't fall over (because it doesn't expect those content preferences being set)? This case has happened, hence this bug to clean up the mess.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

test_bug717433.html has this:

      is(currentDictonary, expected, expected + " expected");
      content.removeEventListener('load', loadListener, false);

      // Remove the fake en-GB dictionary again, since it's otherwise picked up by later tests.
      hunspell.removeDirectory(en_GB);

      // Reset the preference, so the last value we set doesn't collide with the next test.
      Services.prefs.setCharPref("spellchecker.dictionary", "");
      SimpleTest.finish();

Is something wrong with that?

SpecialPowers.pushPrefEnv is a cleaner way to do this though:
https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=pushprefenv

Comment 6

[Jorg K (CEST = GMT+2)]

Yes, sadly. If you look at comment #2 you can see that this test sets the content preference to "en-US". Removing the "en-GB" dictionary and clearing the preference will *not* remove the content preference. If you remove a dictionary which is referenced by a content preference, then there is some clean-up happening and the content preference is removed.

Looking again at comment #2, all the tests which set content preference "en-US" need to be fixed.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1)

What's your feeling on this?

It makes no sense to allow empty content preferences to be written. I debugged it, the content preference really gets written to the database with an empty value.

With this, we'd kill two birds with one stone. Make calling SetCurrentDictionary with an empty string do something useful and also open a door to cleaning up the tests.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2)

Since the SetCurrentDictionary() also sets the preference, we can lose more code.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v3)

Here's the complete fix. Note that test_add_remove_dictionaries.xul doesn't need fixing since it removes the dictionaries for which content preferences were written, so that does the clean-up.

Try run to follow once the try server opens again (currently closed: bug 1204832)

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v4) - Implements option 1) in comment #19

Previous patch wasn't quite right due to cut/paste error and being tricked by an if-statement without braces.

Now we're green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4698e1e0817

Sadly, just when I thought I'm finished, another one popped up: Bug 1193293.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8661349 [details] [diff] [review]
Proposed solution (v4) - Implements option 1) in comment #19

Review of attachment 8661349 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should change our C++ code here. Use pushPrefEnv.

Comment 12

[Jorg K (CEST = GMT+2)]

I agree that changing C++ code in a bug that says "clean up tests" doesn't seem right.

However, I think that SetCurrentDictionary() had the additional task "glued" onto it to write stuff to an mozStorage (SQLite) database. You can call SetCurrentDictionary() with an empty string, and it will deselect any dictionary, however, the additional data sticks like glue, you need to do something completely different to get rid of it.

That doesn't seem right from a design aspect. Why should I use one interface to do something and have to know about the internals and do two things to undo the action.

If you want, please consider this as fixing two bugs:
1) Fix SetCurrentDictionary() to do the right thing when called with an empty string, since it 
   makes no sense to store an empty content preference as it currently does. Preferably clear the
   stored data in this case.
2) Fix these tests.

Look at it some other way: If tomorrow we decide to MozStorage but something else, then we'd have to revisit all the tests.

Would you kindly reconsider your decision?

Comment 13

[Jorg K (CEST = GMT+2)]

I give you another orthogonality argument:

None of the dictionary tests mention content preferences. Instead they do this:
1) Focus an element.
2) Set the dictionary.
3) Reload the page.
4) Check that the setting persisted.

They do NOT do:
1) Focus an element.
2) Set the dictionary.
3) Check that the content preference was correctly written.

I think a test that went:
  SetCurrentDictionary("foobar");
should just have a symmetric clean-up action:
  SetCurrentDictionary("");

To me, this is the cleanest, simplest, safest (and yes, also fastest) solution. Please reconsider.

Comment 14

[Jorg K (CEST = GMT+2)]

Alternatively we could create a function ClearCurrentDictionary() which un-selects the dictionary and clears the content preferences.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it would be better to have a new method ClearCurrentDictionary which just wipes out the content pref. That's better than having a magic parameter value to SetCurrentDictionary. Tests should use pushPrefEnv to save and restore the spellchecker.dictionary pref.

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> I think it would be better to have a new method ClearCurrentDictionary which
> just wipes out the content pref. That's better than having a magic parameter
> value to SetCurrentDictionary.
Fair enough. Although this function would only be used in the tests.

Do you really want to allow a call of SetCurrentDictionary(""); to store an empty value in the content preferences (mozStorage/SQLite)? To my eye, that doesn't make sense. And no user action can ever cause this. The user can only select a dictionary, there is no UI to select "no dictionary".

SetCurrentDictionary() also already clears content preferences, if the chosen dictionary coincides with what the site requires:
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#620
(Line numbers will change today due to a landing).
So in fact, there would be little magic to having "" clear the storage ;-)

> Tests should use pushPrefEnv to save and restore the spellchecker.dictionary pref.
This I do not understand. Tests currently don't know how the dictionary is made to stick. All tests set the dictionary, reload the page and check that by magic the dictionary comes back (see comment #13). This bug is merely about fixing up the tests so they don't leave things set and the next test falls over (a case I experienced and had to work around with the hack of naming the test "test_abug1234567.html" so it ran first).

The preference "spellchecker.dictionary" is an anachronism with little value. In the very early days it determined the spelling, but these days are long gone. Today the preference is set when the user selects a dictionary and serves as a fallback in some cases. See bug 1073827 comment #33 for exact details. Code here, but line numbers will change today due to a landing:
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#796

If you still prefer ClearCurrentDictionary() instead of SetCurrentDictionary(""); all I'd do it implement that and use it accordingly in the four tests I intend to fix, see attachment 8661349 [details] [diff] [review].

I'm sorry about this somewhat academic discussion. None of it will go in front of the user. I just suggested the clean-up, so the next poor guy who adds a test to the spell check area doesn't suffer the same headache that I suffered when my test worked locally (on Windows) but not in bulk (on Linux). I'm glad I didn't follow your suggestion to set up a virtual machine with Linux. Instead I solved the mystery with print-statements, just like in the FORTRAN days ;-)

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: solution with pushPrefEnv (completely wrong: pushPrefEnv is for preferences NOT content preferences)

I think we have discussed enough about this trivial issue. My mistake, sorry.
Here's the solution you wanted using pushPrefEnv.
Try run here, tests run as part of Mochitest "Other":
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec41a8313ad4

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8663153 [details] [diff] [review]
solution with pushPrefEnv (completely wrong: pushPrefEnv is for preferences NOT content preferences)

Didn't work. Hmm. I copied the one line from here:
https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_bug401046.html#75

Comment 19

[Jorg K (CEST = GMT+2)]

Please ignore the previous comments. I got confused. Here is a new version of my answer to your comment #15:

In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> I think it would be better to have a new method ClearCurrentDictionary which
> just wipes out the content pref. That's better than having a magic parameter
> value to SetCurrentDictionary.
Fair enough. Although this function would only be used in the tests.

Do you really want to allow a call of SetCurrentDictionary(""); to store an empty value in the content preferences (mozStorage/SQLite)? To my eye, that doesn't make sense. And no user action can ever cause this. The user can only select a dictionary, there is no UI to select "no dictionary".

SetCurrentDictionary() also already clears content preferences, if the chosen dictionary coincides with what the site requires:
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#620
(Line numbers will change today due to a landing, look for "Clearing content preferences").
So in fact, there would be little magic to having "" clear the storage ;-)

> Tests should use pushPrefEnv to save and restore the spellchecker.dictionary pref.
OK, you want me to use
SpecialPowers.pushPrefEnv({'clear': [['spellchecker.dictionary']]}, SimpleTest.finish);
instead of
Services.prefs.setCharPref("spellchecker.dictionary", "");

That won't be necessary:
Either I can convince you that SetCurrentDictionary("") will clear the content preference *and* the preference, or we go with ClearCurrentDictionary() to do both actions.

Sorry about this lengthy discussion about a trivial issue that will never go in front of the user. Implementing ClearCurrentDictionary() would have taken less time that this discussion.

But to me, we should determine what SetCurrentDictionary("") should do. Currently it writes an empty content preference and that to me makes no sense. So I'd like to restate the options:
1) Make SetCurrentDictionary("") clear preference and content preference (my initial proposal).
2) Implement ClearCurrentDictionary() and have it clear both. Leave SetCurrentDictionary("") as is
   writing an empty content preference.
3) Implement ClearCurrentDictionary() and have it clear both and change SetCurrentDictionary() to reject
   empty input.
4) Implement ClearCurrentDictionary() and have it clear both and change SetCurrentDictionary() to call
   ClearCurrentDictionary() when given empty input (in which case we could just do option 1)).

I'll gladly implement 1) 3) or 4). I don't like option 2) since I think we should fix SetCurrentDictionary() to do something meaningful when receiving empty input.

What do you think?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jorg K (GMT+2) from comment #19)
> Sorry about this lengthy discussion about a trivial issue that will never go
> in front of the user. Implementing ClearCurrentDictionary() would have taken
> less time that this discussion.

Code clarity is important whether or not it affects what goes in front of the user.

> But to me, we should determine what SetCurrentDictionary("") should do.
> Currently it writes an empty content preference and that to me makes no
> sense. So I'd like to restate the options:
> 1) Make SetCurrentDictionary("") clear preference and content preference (my
> initial proposal).
> 2) Implement ClearCurrentDictionary() and have it clear both. Leave
> SetCurrentDictionary("") as is
>    writing an empty content preference.
> 3) Implement ClearCurrentDictionary() and have it clear both and change
> SetCurrentDictionary() to reject
>    empty input.
> 4) Implement ClearCurrentDictionary() and have it clear both and change
> SetCurrentDictionary() to call
>    ClearCurrentDictionary() when given empty input (in which case we could
> just do option 1)).
> 
> I'll gladly implement 1) 3) or 4). I don't like option 2) since I think we
> should fix SetCurrentDictionary() to do something meaningful when receiving
> empty input.

Preferably no-one should call SetCurrentDictionary with an invalid dictionary name. So I suggest we add ClearCurrentDictionary, make SetCurrentDictionary assert when called with an empty string or other invalid dictionary name, and fix callers to stop them from doing that.

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> > 3) Implement ClearCurrentDictionary() and have it clear both and change
> > SetCurrentDictionary() to reject empty input.

> Preferably no-one should call SetCurrentDictionary with an invalid
> dictionary name. So I suggest we add ClearCurrentDictionary, make
> SetCurrentDictionary assert when called with an empty string or other
> invalid dictionary name, and fix callers to stop them from doing that.

Looks like you opted for option 3).

I was in the process of implementing this and found this:
https://dxr.mozilla.org/mozilla-central/source/editor/txtsvc/nsISpellChecker.h?offset=100#113

/**
 * Tells the spellchecker to use a specific dictionary.
 * @param aDictionary a string that is in the list returned
 * by GetDictionaryList() or an empty string. If aDictionary is
 * empty string, spellchecker will be disabled.
 */
NS_IMETHOD SetCurrentDictionary(const nsAString &aDictionary) = 0;

So it seems like passing in an empty string is desired behaviour.

So we have to options:
1) Leave the old behaviour and fix it, so content preferences are removed.
   (My initial proposal, see attachment 8661349 [details] [diff] [review]).
2) Go ahead and change the established semantics of SetCurrentDictionary() and add
   ClearCurrentDictionary(). As far as I can see SetCurrentDictionary() is not called
   with an empty string in the code base, but changing the behaviour might upset
   some add-ons.

Looking at nsISpellChecker.h I've noticed that I forgot to remove CheckCurrentDictionary() (which got removed as part of bug 1193293). Once you decide which option you prefer, I'll present another patch.

Since you mentioned "code clarity", I also suggest to use "TryDictionary()" internally instead of "SetCurrentDictionary()", which then removes the necessity of the RAII object.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good catch. Let's do option 1.

Comment 23

[Jorg K (CEST = GMT+2)]

Thanks.

As I said: I'll submit another patch with some more clean-up (remove CheckCurrentDictionary() from nsISpellChecker.h, switch over to using TryDictiornay() internally and remove the RAII trickery).

Coming up, you'll see it.

Comment 24

[Jorg K (CEST = GMT+2)]

Changed my mind. Doing the clean-up I mentioned, I realised that the fix in bug 1193293 wasn't right. So I had that backed out. Any further clean-up will be done in there.

So we go with this "as is":
1) Fix SetCurrentDictionary() to do the right thing when given an empty string.
2) Fix the tests.

Comment 25

[Jorg K (CEST = GMT+2)]

Dear Sheriff,

Please land bug 1193293 and bug 1204540 together. Please apply the patch from bug 1193293 first.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20624d683ef6

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/20624d683ef6