634626 - test_bug366682 failures because spellchecking is disabled

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

This mochitest is failing, because spellchecking is disabled for Fennec.
I guess the best solution is to adjust the testcase in such a way that it detects if spellchecking is enabled, and when it isn't, that it doesn't carry out the tests at all.

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

This fixes it. I've tested and it seems to work.

Comment 2

[Martijn Wargers (dead)]

I get a "todo | [[Simpletest.report()]] No checks actually run" message, though.

It might be that Serge considers this bad practice, considering bug 496443. Serge, how should I avoid that message?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 512831 [details] [diff] [review]
patch

this patch looks fine.  Does it work with e10s (browser.tabs.remote=true)?

After looking at the test file, it seems that we are basically doing nothing for this test if spellcheck is not on.

Can we turn on spell check inside of fennec?  Would we want to remove this file in general from a mobile build inside the makefile?  

I just want to make sure we solve the problem the right way, and if editing the test case itself (which I think is a good way to do it) is the approach we want, then this patch is good.  I would add something in the log file to indicate we are skipping this test because spellcheck is not turned on if we do skip it.

Comment 4

[Martijn Wargers (dead)]

(In reply to comment #3)
> this patch looks fine.  Does it work with e10s (browser.tabs.remote=true)?

Yes, this is on Windows, though. (I'm afraid I'm still stuck on the Tegra board)
 
> After looking at the test file, it seems that we are basically doing nothing
> for this test if spellcheck is not on.

Yes, that's what I mentioned in comment 2.

> Can we turn on spell check inside of fennec?  Would we want to remove this file
> in general from a mobile build inside the makefile?  

Afaik, spellchecking can only be turned on for Fennec, when doing that in about:config. My guess is that at one point, this is getting turned on again in Fennec. At that point, with this patch, the test would automatically work again.
With removing this file in the Makefile, you would have to remember to readd this file.

> I just want to make sure we solve the problem the right way, and if editing the
> test case itself (which I think is a good way to do it) is the approach we
> want, then this patch is good.  I would add something in the log file to
> indicate we are skipping this test because spellcheck is not turned on if we do
> skip it.

Yeah, I was just asking Serge in comment 2 about it. I'll see what he writes about it and write a better fix if he has a good suggestion.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Use todo() when spellCheck is turned off

Same as previous patch, except with is() replaced by todo() and ok() ;-)

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 512860 [details] [diff] [review]
(Av2) Use todo() when spellCheck is turned off

cool!  this addresses my concern from the previous patch.

Comment 7

[Martijn Wargers (dead)]

Thanks, Serge!

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) s/CR+LF/LF/g, Remove ending whitespaces

While here...

Comment 9

[Martijn Wargers (dead)]

Sorry, there are probably some other tests that need the same treatment. I'll try to change those too, tomorrow:
/editor/libeditor/html/tests/test_bug484181.html (View Hg log or Hg annotations)
/editor/libeditor/html/tests/test_bug366682.html (View Hg log or Hg annotations)
/editor/libeditor/html/tests/test_bug432225.html (View Hg log or Hg annotations)
/editor/libeditor/text/tests/test_bug596333.html (View Hg log or Hg annotations)

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 512950 [details] [diff] [review]
(Bv1) s/CR+LF/LF/g, Remove ending whitespaces

hmm, this won't apply with the other patch.  It is just a line ending cleanup for the existing file.  I don't see anything wrong with the patch, just that it will require a new patch for the isspelling todo patch earlier.

Can we make this patch as a second in a series or a superset?

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 512950 [details] [diff] [review]
(Bv1) s/CR+LF/LF/g, Remove ending whitespaces

(In reply to comment #10)
> Can we make this patch as a second in a series or a superset?

My intent is to land this patch (just) before an updated Av2 patch.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 512950 [details] [diff] [review]
(Bv1) s/CR+LF/LF/g, Remove ending whitespaces

ok great.

Comment 13

[Martijn Wargers (dead)]

Attachment: patchv2

This fixes the instances mentioned in comment 9.

Comment 14

[(no longer active)]

Comment on attachment 513080 [details] [diff] [review]
patchv2

r- because this makes the test pass if the pref gets disabled for Firefox, which we don't want.

Comment 15

[Martijn Wargers (dead)]

Why not?

Comment 16

[(no longer active)]

Comment on attachment 512950 [details] [diff] [review]
(Bv1) s/CR+LF/LF/g, Remove ending whitespaces

The landing of this patch made me *really* sad.  These types of patches effectively ruin the blame history, are useless, and should not land without going through the proper review requirements.

I'll r- to express my discontent here, but I guess it's too late to fix this now.  :(

Comment 17

[(no longer active)]

(In reply to comment #15)
> Why not?

Because we want to catch this type of breakage in Firefox.  ie., if somebody makes that pref value false, we want the unit tests to fail.

Comment 18

[(no longer active)]

Comment on attachment 512860 [details] [diff] [review]
(Av2) Use todo() when spellCheck is turned off

r- because of the mentioned reason.

Comment 19

[Martijn Wargers (dead)]

(In reply to comment #17)
> (In reply to comment #15)
> > Why not?
> 
> Because we want to catch this type of breakage in Firefox.  ie., if somebody
> makes that pref value false, we want the unit tests to fail.

Why would you want the unit tests to fail in that case?

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Ehsan,

In Fennec these tests fail and have the pref off.  All automation is run with a clean fresh profile including all the hard requirements.  If we decide to run in the wild, the tests should be smart enough not to throw a lot of errors just because some settings were tweaked.

Is there something else I am overlooking?

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> r- because this makes the test pass if the pref gets disabled for Firefox,
> which we don't want.

Why don't you add a Firefox specific test to check the pref value, if that's explicitly what you want to check?
Forcing Core tests to depend on Firefox likings isn't good at all (for all other apps), I think.

Comment 22

[(no longer active)]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > Why not?
> > 
> > Because we want to catch this type of breakage in Firefox.  ie., if somebody
> > makes that pref value false, we want the unit tests to fail.
> 
> Why would you want the unit tests to fail in that case?

Because we want to catch the case where someone changes the default value for that pref in Firefox.  We want the tests to fail in that case.

Comment 23

[(no longer active)]

(In reply to comment #20)
> Ehsan,
> 
> In Fennec these tests fail and have the pref off.  All automation is run with a
> clean fresh profile including all the hard requirements.  If we decide to run
> in the wild, the tests should be smart enough not to throw a lot of errors just
> because some settings were tweaked.
> 
> Is there something else I am overlooking?

Yes, I understand.  The right solution is to detect whether we're in Fennec and expect the default value of the pref to be false if the test is run in Fennec.

Comment 24

[(no longer active)]

(In reply to comment #21)
> (In reply to comment #14)
> > r- because this makes the test pass if the pref gets disabled for Firefox,
> > which we don't want.
> 
> Why don't you add a Firefox specific test to check the pref value, if that's
> explicitly what you want to check?

The owner of this bug can do that.  If we do have such a Firefox specific test, then my point no longer applies, but that's not really needed (see comment 23).

> Forcing Core tests to depend on Firefox likings isn't good at all (for all
> other apps), I think.

Nobody is talking about Firefox specific things.  We want every Gecko based app to support spell checking unless there is a good reason to the contrary.  Fennec has one I assume, so it should be exempted explicitly.  Not the other way around.

Comment 25

[Martijn Wargers (dead)]

I chatted with Ehsan on irc about this.
The solution we came up is that in the "if (!spellcheckOn)" check, we add another check that checks if Fennec is running, and if Fennec is running, you get the todo() result, otherwise you get a fail result (for any other Gecko based application).

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

I know you guys already decided this, but I think there should be a separate test in browser (i.e. Firefox specific) that makes sure the default pref is whatever we want it to be for Firefox.

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

there has been random talk of letting end users run mochitests on their systems and if they have a pref misconfigured it will be hard to tell what is going on.  This scenario is probably a bigger issue, I am just glad we are making some progress to get more tests green (or not red) on Fennec.

Comment 28

[(no longer active)]

(In reply to comment #26)
> I know you guys already decided this, but I think there should be a separate
> test in browser (i.e. Firefox specific) that makes sure the default pref is
> whatever we want it to be for Firefox.

Firefox is not the exception here.  Fennec is.  So on the second thought, we shouldn't do *anything* specific to Firefox here.  See comment 25 about what needs to happen here.

Comment 29

[(no longer active)]

(In reply to comment #27)
> there has been random talk of letting end users run mochitests on their systems
> and if they have a pref misconfigured it will be hard to tell what is going on.

FWIW, this is not the goal of this bug at all, and if some day we get there, we should discuss that part in its own bug.  Hint: getting there requires *lots* of effort, as our tests make very specific assumptions which are only going to be true in the test suite environment.

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #28)
> (In reply to comment #26)
> > I know you guys already decided this, but I think there should be a separate
> > test in browser (i.e. Firefox specific) that makes sure the default pref is
> > whatever we want it to be for Firefox.
> 
> Firefox is not the exception here.  Fennec is.  So on the second thought, we
> shouldn't do *anything* specific to Firefox here.  See comment 25 about what
> needs to happen here.

my reasoning is that the default for a given pref is an app level preference and should not be enforced in platform tests

Comment 31

[(no longer active)]

(In reply to comment #30)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > I know you guys already decided this, but I think there should be a separate
> > > test in browser (i.e. Firefox specific) that makes sure the default pref is
> > > whatever we want it to be for Firefox.
> > 
> > Firefox is not the exception here.  Fennec is.  So on the second thought, we
> > shouldn't do *anything* specific to Firefox here.  See comment 25 about what
> > needs to happen here.
> 
> my reasoning is that the default for a given pref is an app level preference
> and should not be enforced in platform tests

No, we have platform level prefs which apps can _override_.  Spellchecking is enabled by default in core: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#376>.  Fennec disables it: <http://mxr.mozilla.org/mobile-browser/source/app/mobile.js#192>.  So, any application which is not Fennec will get spellchecking by default, so I don't want to special case anything for Firefox.

Comment 32

[Martijn Wargers (dead)]

(In reply to comment #31)
> No, we have platform level prefs which apps can _override_.  Spellchecking is
> enabled by default in core:
> <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#376>.

Why is Firefox setting the "layout.spellcheckDefault" then?
http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#631
Afaict, it shouldn't be doing that, should it? Since it is already set on a platform level?

I'm trying to find out where the decision was made that this should be turned on at the platform level. I found bug 302050, comment 29 where the developers where leaning on turning it off at the platform level.
Could anybody give me a link of a page that describes this should have been turned on at the platform level?

Comment 33

[(no longer active)]

(In reply to comment #32)
> (In reply to comment #31)
> > No, we have platform level prefs which apps can _override_.  Spellchecking is
> > enabled by default in core:
> > <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#376>.
> 
> Why is Firefox setting the "layout.spellcheckDefault" then?
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#631
> Afaict, it shouldn't be doing that, should it? Since it is already set on a
> platform level?

It's just setting it to the default value.  Why does it matter?

> I'm trying to find out where the decision was made that this should be turned
> on at the platform level. I found bug 302050, comment 29 where the developers
> where leaning on turning it off at the platform level.
> Could anybody give me a link of a page that describes this should have been
> turned on at the platform level?

There is no such page.  That's just how the code works right now.  Still, I don't see why this is important.  What's wrong with just testing this correctly for both Fennec and Firefox?

Comment 34

[Martijn Wargers (dead)]

Ok, never mind, Firefox setting the "layout.spellcheckDefault" pref is probably because it's in the advanced prefs pane:
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/advanced.xul#73

And I guess the decision to turn this on on a platform level has been made at bug 339127, comment 17:
"
* Editor will enable spellchecking if the user has forced it on, or if the
element says it should be on and the user has not forced it off (either locally
or globally through layout.spellcheckDefault).  Note that checking the element
(to get the UA default) required some irritating changes to nsIContent et. al
"

Comment 35

[(no longer active)]

I still think that the way to go here would be comment 25...

Comment 36

[Martijn Wargers (dead)]

Attachment: patchv3

This is as done as suggested in comment 25.
This doesn't work actually, because in Fennec ipc is enabled and this gives an error when querying for nsIXULAppInfo.

So what needs to be done in order for this to get working is, you would need a new api that uses specialpowers.js to message to chrome content to query the appInfo.id. That's quite a hassle, I'll try and see if I can get it working.
Anyway, the querying for prefs in these mochitests need to be adjusted too, using the specialpowers.js api.

Comment 37

[Josh Matthews [:jdm]]

>This doesn't work actually, because in Fennec ipc is enabled and this gives an
>error when querying for nsIXULAppInfo.

Huh, I wonder if this is something we should fix. Filed bug 640700.

Comment 38

[Martijn Wargers (dead)]

Attachment: add getAppID for specialpowers

I don't know if this is desirable for specialpowers. Anyway, I would need this to follow the way of comment 25.

Comment 39

[Martijn Wargers (dead)]

Attachment: patchv4

Comment 40

[(not currently active) Ted Mielczarek]

Comment on attachment 519904 [details] [diff] [review]
add getAppID for specialpowers

This looks fine, just two nits:

>diff --git a/testing/mochitest/specialpowers/components/SpecialPowersObserver.js b/testing/mochitest/specialpowers/components/SpecialPowersObserver.js
>--- a/testing/mochitest/specialpowers/components/SpecialPowersObserver.js
>+++ b/testing/mochitest/specialpowers/components/SpecialPowersObserver.js
>@@ -80,16 +80,17 @@ SpecialPowersObserver.prototype = {
>+      case "SPappinfo":
>+        var appinfo = Components.classes["@mozilla.org/xre/app-info;1"]
>+                         .getService(Components.interfaces.nsIXULAppInfo);
>+        return(appinfo.ID);
>+        break;

You don't need the parentheses in the return line (I know we have them in other lines, they're not necessary there either).

>diff --git a/testing/mochitest/specialpowers/content/specialpowers.js b/testing/mochitest/specialpowers/content/specialpowers.js
>--- a/testing/mochitest/specialpowers/content/specialpowers.js
>+++ b/testing/mochitest/specialpowers/content/specialpowers.js
>@@ -115,16 +115,19 @@ var SpecialPowers = {
>                                                            listener,
>                                                            false);
>   },
>   isBackButtonEnabled: function(window) {
>     return !this._getTopChromeWindow(window).document
>                                       .getElementById("Browser:Back")
>                                       .hasAttribute("disabled")
>   },
>+  getAppID: function() {
>+    return sendSyncMessage('SPappinfo', {})[0];
>+  },
> }

Can you add a simple sanity check test for this in http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/test_SpecialPowersExtension.html?force=1 ? Something like isnot("", SpecialPowers.getAppID()), just to make sure that the value gets set.

Also, for bonus points you could make this a getter instead, so you could just say "SpecialPowers.AppID", something like:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/mq/file/1a5c5c5ecf2f/de-privilege#l171

But that's just an option.

Comment 41

[(not currently active) Ted Mielczarek]

But um, looking at your test usage, testing the App ID seems like the wrong thing to do. I fully disagree with Ehsan here, you shouldn't have to resort to checking the App ID inside of tests.

If Ehsan wants a sanity check test that asserts that spell checking is enabled for Firefox, he should write a Mochitest in browser/ that simply checks the pref value and fails if it's not set, not force you to put awful hacks like this into tests.

Comment 42

[(not currently active) Ted Mielczarek]

(In reply to comment #26)
> I know you guys already decided this, but I think there should be a separate
> test in browser (i.e. Firefox specific) that makes sure the default pref is
> whatever we want it to be for Firefox.

Yes, this. Why did nobody listen to this?

Comment 43

[Martijn Wargers (dead)]

(In reply to comment #41)
> If Ehsan wants a sanity check test that asserts that spell checking is enabled
> for Firefox, he should write a Mochitest in browser/ that simply checks the
> pref value and fails if it's not set, not force you to put awful hacks like
> this into tests.

If I understand correctly, Ehsan wants a sanity check test that asserts that spell checking is not disabled for toolkit applications other than Firefox.

I guess I could do that check in browser/base/content/test/ instead. But I would still need the SpecialPowers.getAppID() code for that.

Comment 44

[(no longer active)]

OK, let's reach a middle ground here.  Let's get a test in editor/libeditor/base/tests which asserts that the pref value is true, and just that.  Then exclude that test from Fennec in Makefile.in (don't use the app ID), so that any Gecko application except for Fennec would run it, and then modify the rest of the spellchecking tests to check layout.spellcheckDefault, and just bail out if it's false.

Also, there are a lot of spellchecker tests in http://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/ too.  Although I'm not sure if you care about reftests at this point.

Comment 45

[Brad Lassey [:blassey] (use needinfo?)]

Ehsan, at the root of it, the pref is an app preference and should only be tested by app specific tests. The platform should not be testing for the pref.

Comment 46

[(no longer active)]

(In reply to comment #45)
> Ehsan, at the root of it, the pref is an app preference and should only be
> tested by app specific tests. The platform should not be testing for the pref.

Please see comment 31.  The default platform behavior is to spell check editable fields, and I want our tests to fail if somebody breaks that.  I also want to do that as cleanly as possible.  I'm open to suggestions if you think there's a better way to ensure that than what I suggested in comment 44.

Comment 47

[Martijn Wargers (dead)]

I guess this with the move to use SpecialPowers, these tests need to move to chrome tests, probably.

Comment 48

[(no longer active)]

(In reply to comment #47)
> I guess this with the move to use SpecialPowers, these tests need to move to
> chrome tests, probably.

I agree.

Comment 49

[Martijn Wargers (dead)]

Attachment: move test_bug36682 to mochitest chrome (patch v5)

I haven't tested this with, because my build is outdated (still had the packed.js thing in there, I already tested with the packed.js thing, it worked fine then, so I'm confident this is working also, but I'll have to retest).

I guess the first suggestion in comment 44 still has to be done, right?

Comment 50

[Martijn Wargers (dead)]

Attachment: move test_bug36682 to mochitest chrome , added layout.spellcheckDefault pref test

Ok, this adds the layout.spellcheckDefault pref test. This works fine in Firefox mochitest and mochitest-chrome, I haven't tested on Fennec yet (they should not run at all, there).

Comment 51

[(no longer active)]

Comment on attachment 556843 [details] [diff] [review]
move test_bug36682 to mochitest chrome , added layout.spellcheckDefault pref test

>+SimpleTest.waitForExplicitFinish();
>+addLoadEvent(runTest);
>+
>+function runTest()
>+{
>+  is(SpecialPowers.getIntPref("layout.spellcheckDefault"), 1, "Check if the layout.spellcheckDefault pref is turned on");
>+  SimpleTest.finish();
>+}

No need to wait for the load event, merely the |is| call should be enough.  Looks great, r=me with this nit addressed.

Thanks!

Comment 52

[Martijn Wargers (dead)]

Attachment: move test_bug36682 to mochitest chrome , added layout.spellcheckDefault pref test v2

With nit of comment 51 addressed.

Comment 53

[Ed Morley [:emorley]]

Missing commit message / author, so removing checkin-needed.

I'd add them myself, but given the 9 obsolete patches & 50+ comments here, not sure mine would accurately summarise the current approach / reasoning for it, without me having to read all 50 comments.

Also might be worth fixing your hgrc so the author gets added automatically. Only takes 10 seconds to change, but saves everyone else lots of time in the long run.

Thanks :-)

Comment 54

[Martijn Wargers (dead)]

(In reply to Ed Morley [:edmorley] from comment #53)
> Missing commit message / author, so removing checkin-needed.

I'm the author, you can add my name, thanks.

> I'd add them myself, but given the 9 obsolete patches & 50+ comments here,
> not sure mine would accurately summarise the current approach / reasoning
> for it, without me having to read all 50 comments.

Ok, brief summary:
test_bug366682.html is failing on Fennec, because spellchecking is disabled there.
There were various solutions to the problem:
- Disable the test from inside the Makefile.in for Fennec
- Fix the test in such a way that it isn't run for agents that don't have the spellchecking pref set.
- Move the test to chrome tests and add a check that checks that the spellhecking pref is turned on and disable that test for Fennec.
After much debate, we opted for the 3rd solution. However, this is not the optimal solution. It was the middleground that was reached. I'll file a follow-up bug for the optimal solution, as soon as this bug is fixed (although, I have to admit, I don't know what the optimal solution is myself).

> Also might be worth fixing your hgrc so the author gets added automatically.
> Only takes 10 seconds to change, but saves everyone else lots of time in the
> long run.

How do I fix my hgrc?
 
> Thanks :-)

Thank you for checking in!

Comment 55

[Boris Zbarsky [:bzbarsky]]

> How do I fix my hgrc?

 [ui]
 username = First Middle Last <email>

(or whatever string you want to show up as author).

Also:

 [defaults]
 qnew = -U

so that all mq patches automatically add the author bit.

For the checkin comment, I recommend either putting it directly in the patch (using hg qref) or explicitly putting the text you want as the checkin comment in the bug.

Comment 56

[(no longer active)]

More precisely, hg qnew -e will let you edit  the commit message.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Or hg qref -e, yes.

Comment 58

[Martijn Wargers (dead)]

Attachment: 560306: move test_bug36682 to mochitest chrome , added layout.spellcheckDefault pref test v3

Ok, I guess this is what you want, I found this patch inside:
C:\mozilla-build\fennec_trunk\.hg\patches
And I believe this is the format that is wanted.

Comment 59

[Ed Morley [:emorley]]

In my queue with a few other bits that are being sent to try first and then onto inbound :-)

Comment 60

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=87ba0b8cbf99

https://hg.mozilla.org/integration/mozilla-inbound/rev/b063f02ccd43

Comment 61

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b063f02ccd43

Comment 62

[Martijn Wargers (dead)]

Thanks for the patience, everyone.