Closed Bug 629734 Opened 14 years ago Closed 14 years ago

Integrate hunspell's tests into our test suite

Categories

(Core :: Spelling checker, defect)

Product:
Core
Component:
Spelling checker
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla5

People

(Reporter: ehsan.akhgari, Assigned: khuey)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Import hunspell test suite
351.27 KB, patch
Details | Diff | Splinter Review
Integrate hunspell test suite as an xpcshell test.
12.11 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Add todo functionality to xpcshell.
6.93 KB, patch
ted
: review+
Details | Diff | Splinter Review 
We need to integrate hunspell's tests into our test suite to make sure that we at least run those tests on every check-in...

Nemeth, could you please guide me on how you guys test stuff in hunspell?  And how do you think we can integrate your tests?
The automatic (targeted by make check) tests are in the root of the tests subdirectory of the Hunspell distribution. See tests/test.sh for the details. The tests can be executed individually, too:

eg. 

cd tests
./base.test

VALGRIND=memcheck ./base.test

The base.test tests Hunspell with the relevant test dictionary (base.aff and base.dic) on the accepted (base.good) and deprecated (base.wrong) input words. There are optionally other tests for suggestion (eg. base.sug) and morphological analysis (eg. circumfix.morph).

Sometimes the tests change with the code, for example with the optimization of the suggestion algorithm it had to modify some of the *.sug test files.

By the way, OpenOffice.org source tree contains the original Hunspell distribution, and Hunspell is patched, compiled, maybe tested during the compilation of OpenOffice.org. Maybe a similar method or automation could help the update process.
Thanks a lot, Nemeth!  This is very useful.  :-)

Just one more question.  Is it also possible to run the tests from the spellchecking bindings that we have in mozilla too?  I mean, is it desirable for us to try to convert the tests into HTML files and make sure that what you guys are testing also passes from within the browser?  (I don't know how easy/possible this is, really...)

Kyle, do you know how easy this would be to integrate into our C++ tests?  (I've never fully understood how they work).
Without seeing the actual test code, it's hard to judge, but I doubt it would be difficult.
(In reply to comment #3)
> Without seeing the actual test code, it's hard to judge, but I doubt it would
> be difficult.

Nemeth, is the hunspell code browseable on the web on a CVS-web view or something?
Yes, though SF's CVS servers are down currently.
http://hunspell.cvs.sourceforge.net/hunspell

They are also packaged in the source tarballs.
Comment on attachment 510196 [details] [diff] [review]
Integrate Hunspell test suite.

This is a first pass at integrating the Hunspell test suite into our automated test suites.  The Hunspell tests suites want to run against the hunspell binaries which we don't actually build (we build the static version) so I've created an xpcshell harness that bridges the test suites.  This patch doesn't integrate the suggestion tests.

Two notes:
1. Adding the function to load dictionaries from a directory to the IDL is a little hacky; there might be a better way to do this but I wanted to get it working.
2. There's some hackiness involved with reading the files off of the disk in the proper encoding.  Some of the tests fail and I'm not sure if that's due to bugs in our code, bugs in the test, or issues with character encodings.

Comments welcome.
(This applies on top of the hunspell 1.3.1 patch RyanVM has)
Comment on attachment 510196 [details] [diff] [review]
Integrate Hunspell test suite.

> NS_INTERFACE_MAP_BEGIN(mozHunspell)
>+  NS_INTERFACE_MAP_ENTRY(mozISpellCheckingEngine_MOZILLA_2_0_BRANCH)

No need for the new interfaces, as we're not going to take hunspell 1.3.1 on trunk, so this will probably land after 2.0...

>-#ifdef DEBUG_bsmedberg
>     printf("Adding dictionary: %s\n", NS_ConvertUTF16toUTF8(dict).get());
>-#endif

Did you remove these by mistake?

The xpcshell test looks good, and I think that's what we want to do anyways (testing our own binding to hunspell too).  I didn't read much of the other parts of the patch.  I suggest you split it into two parts, the parts dropped directly from hunspell, and the parts you wrote yourself, for easier reviews once it's ready.

But overall, this looks wonderful!  Thanks for taking this.
Attachment #510196 - Flags: feedback?(ehsan) → feedback+
(In reply to comment #9)
> Comment on attachment 510196 [details] [diff] [review]
> Integrate Hunspell test suite.
> 
> > NS_INTERFACE_MAP_BEGIN(mozHunspell)
> >+  NS_INTERFACE_MAP_ENTRY(mozISpellCheckingEngine_MOZILLA_2_0_BRANCH)
> 
> No need for the new interfaces, as we're not going to take hunspell 1.3.1 on
> trunk, so this will probably land after 2.0...

OK, I saw in one of the other bugs Ryan suggesting that we take the upgrade for 2.0, so I covered my bases ;-)

> >-#ifdef DEBUG_bsmedberg
> >     printf("Adding dictionary: %s\n", NS_ConvertUTF16toUTF8(dict).get());
> >-#endif
> 
> Did you remove these by mistake?

In the sense that I didn't mean for it to end up in the final patch, yes.
 
> The xpcshell test looks good, and I think that's what we want to do anyways
> (testing our own binding to hunspell too).

Agreed.

> I didn't read much of the other
> parts of the patch.  I suggest you split it into two parts, the parts dropped
> directly from hunspell, and the parts you wrote yourself, for easier reviews
> once it's ready.

Sure

> But overall, this looks wonderful!  Thanks for taking this.

Np.
Attachment #510196 - Attachment is obsolete: true
Attachment #510196 - Flags: feedback?(ryanvm)
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Whiteboard: [post 2.0]
I had to disable several more tests this time around since we backed the hypen patch out.
Comment on attachment 513307 [details] [diff] [review]
Integrate hunspell test suite as an xpcshell test.

I don't understand the changes to extensions/spellcheck/hunspell/tests/checkcompoundrep.wrong.

Could you make the currently failing tests "todo", so that we can catch them when we fix something in hunspell?

Other than that, looks great to me.
Attachment #513307 - Flags: review?(ehsan)
Oh, it looks like that one just didn't end up in the first one for some reason ...

I'll look into a "todo" mechanism, not sure that xpcshell has one.
(In reply to comment #15)
> Oh, it looks like that one just didn't end up in the first one for some reason
> ...
> 
> I'll look into a "todo" mechanism, not sure that xpcshell has one.

Glancing over the code here <http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js>, it seems that it doesn't.  But I think we *really* want this, and adding "todo" support in xpcshell doesn't sound very hard... ;-)
Comment on attachment 513907 [details] [diff] [review]
Integrate hunspell test suite as an xpcshell test.

Comments addressed
Attachment #513907 - Flags: review?(ehsan)
Comment on attachment 513907 [details] [diff] [review]
Integrate hunspell test suite as an xpcshell test.

Looks great!

Can we also run the suggestion tests?  It's not clear to me why you're skipping them...
Attachment #513907 - Flags: review?(ehsan) → review+
Hooking them up is harder so I skipped it for the moment.
(In reply to comment #22)
> Hooking them up is harder so I skipped it for the moment.

OK.  So can you please file a follow-up bug for that?  :)
I fully intended to do it here.
Comment on attachment 513908 [details] [diff] [review]
Add todo functionality to xpcshell.

>diff --git a/testing/xpcshell/head.js b/testing/xpcshell/head.js
>--- a/testing/xpcshell/head.js
>+++ b/testing/xpcshell/head.js
>@@ -1,8 +1,9 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

I'm not sure Java mode is what we actually want, but I'll just change it if it causes me problems.

>+function do_throw_todo(text, stack) {
>+  if (!stack)
>+    stack = Components.stacker.caller;

Components.stack


Looks fine otherwise. Makes me wish we had bug 468196 fixed, to avoid proliferating a whole new set of APIs.
Attachment #513908 - Flags: review?(ted.mielczarek) → review+
Kyle, are these patches based on hunspell 1.3.1?  I am updating hunspell to 1.3.1 in bug 620626 (just landed on cedar) and I was wondering if you've tested the tests here with that version?
Yes, these are based on hunspell 1.3.1.
This turned the cedar builds for Windows red yesterday, so I landed another patch in trying to fix the problem <http://hg.mozilla.org/projects/cedar/rev/3f5c91411c19>, but the windows builds are still red:

http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1301185939.1301188388.23555.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1301185939.1301188235.22322.gz

So, I backed all patches in this bug out in this changeset: <http://hg.mozilla.org/projects/cedar/rev/b7a26dac40ba>
Whiteboard: fixed-in-cedar → not-ready
Yeah, it was supposed to be foo/, not foo/* :-P

Landed as:

http://hg.mozilla.org/mozilla-central/rev/14033ede4ba6
http://hg.mozilla.org/mozilla-central/rev/f99b6caaaff9
http://hg.mozilla.org/mozilla-central/rev/1c1c1a81419e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: not-ready
Target Milestone: --- → mozilla2.2
Blocks: 652364
Depends on: 656944
Blocks: 658710
Depends on: 672040
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: