Test failure : apitest, bigoaddvalue

[Charlie Hull]

Test fails as follows:

Running test: bigoaddvalue... Adding a document with 5000 values took 0 seconds Adding a document with 50000 values took 0.063 seconds .\api_wrdb.cc:1927: ((time_10N) < (time_N * ALLOWED_FACTOR)) Expected time_10N' to be less than than time_N * ALLOWED_FACTOR': were 0.063 and 0

Probably due to Windows not giving us decent timing info.

[Richard Boulton]

Suggestions for better functions to get timing info are very welcome. The current implementation is in common/omtime.h - I might try a rewrite of that entire file at some point for copyright cleanness, actually; it's not the ideal interface, anyway.

[Olly Betts]

I suggest that for now we skip instead of fail if we get both times being equal and exact integers.

There is a better function to use - I noted it on IRC a while ago:

@1214350526 <ojwb!i=olly@…> just that windows will be using time() to read the current time @1214350533 <ojwb!i=olly@…> so will have second granularity @1214350812 <ojwb!i=olly@…> ​http://msdn.microsoft.com/en-us/library/ms724397(VS.85).aspx @1214350817 <ojwb!i=olly@…> that seems the function to use @1214350848 <ojwb!i=olly@…> no support for NT or the 9x family though @1214350869 <ojwb!i=olly@…> which isn't a big deal, but it would be nice to at least fall back to using time()

But as it's not available on NT or win 9x, and other platforms might be missing a better time function (or we may not detect it), skipping as described above would be good anyway.

I'm not sure it's worthwhile adding support for above to the existing OmTime - we're testing for O() behaviour of portable code here, which I can't see varying by platform. And as you say it's worth rewriting OmTime at some point.

Hmm, in fact I think I might have a partial rewrite somewhere already - I certainly remember thinking about a better API myself. It's not anywhere obvious on atreus or the laptop, but perhaps it's on one of the drives from the dead dev box. I'll look later.

[Olly Betts]

Actually, both equal and integers is the wrong test here I think.

If the shorter test takes 0.0 seconds, then the timer granularity is too coarse (even if the timer has better granularity than 1 second). So I've added a check which skips if that is the case to SVN trunk.

Charlie: can you test this on Windows? If this fixes the test failure, we should backport it to the 1.0 branch.

[Richard Boulton]

Note about why Charlie hasn't tested this on Windows yet: trunk doesn't currently compile on windows, pending fixing of bug #303. If this is holding up 1.0.9, we could either make a patch from your commit to trunk to fix this, and test that fix directly against branches/1.0 on windows, or just note that this test will fail harmlessly on windows in the release notes.

[Olly Betts]

It is holding up 1.0.9, but it's not the only thing that is currently.

The fix on trunk would also fix the case of completing the test in less than the timer resolution on any platform, so at least theoretically it's not just a fix for Microsoft Windows. But it would be reassuring to check it actually fixes it where we have a reproducer.

[Olly Betts]

Backported patch

[Olly Betts]

Can you try the attached patch?

With it you should get test bigoaddvalue skipped (unless your box is slow enough that the 5000 value run takes a second or more):

./apitest bigoaddvalue

[Charlie Hull]

I'm confused; is this patch for branch/1.0 or trunk? If so it will have to wait until I have a fix for #303 (hopefully later this week).

[Richard Boulton]

The patch is already applied on trunk, so I assume it's for the 1.0 branch.

When you do get trunk compiled, it would be good to know if this bug is fixed "out-of-the-box", too.

[Olly Betts]

Yes, the patch I attached is backported (hence the description I gave it!)

[Charlie Hull]

I had trouble applying the patch, as SVN reports it's out of date w.r.t. branch/1.0; however manually adding the new lines appears to work. Improved patch attached, plus another patch to the Win32 makefiles for tests - the way we run collate-apitest appears to have changed recently.

[Charlie Hull]

Working patch for current SVN on 23/10/08

[Charlie Hull]

Patch for build files to cope with new way of running collate-apitest

[Olly Betts]

Um, what exactly do you mean by "SVN reports it's out of date w.r.t. branch/1.0"?

SVN shouldn't be relevant here - you should be using the patch utility to apply patches. This patch applies fine to my clean 1.0 tree (and I did of course test this before attaching it):

olly@atreus:~/svn/xapian/xapian-core/tests$ svn up
At revision 11579.
olly@atreus:~/svn/xapian/xapian-core/tests$ svn info|grep URL
URL: svn+userv:///xapian/branches/1.0/xapian-core/tests
olly@atreus:~/svn/xapian/xapian-core/tests$ svn status api_wrdb.cc
olly@atreus:~/svn/xapian/xapian-core/tests$ patch -p1 < bug300.patch 
patching file api_wrdb.cc
Hunk #1 succeeded at 1922 (offset -106 lines).

But your change only differs by whitespace, so I suspect it's probably end of line issues. I've applied the first patch to the branch (since it matches the change in trunk and the whitespace is tidier in that version).

[Olly Betts]

Indeed collate-apitest has changed.

Also collate-test in trunk changed similarly, so you'll need to fix this in trunk too in a similar way.

And please write ChangeLog entries (having a ChangeLog which isn't reliably updated is worse than not having one IMO).

But anyway, this is a different issue, so I'm going to close this ticket now as the issue it was opened for is now fixed in both trunk and 1.0. Please attach the revised collate-*test patches to a new ticket.

[Charlie Hull]

Sorry to dig this up again, but I wanted to clarify my patching problems: what seems to be happening is that the patch line numbers don't match the source code; for an example see your second patch for #308 and the current version of queryparsertest.cc (revision 11503 according to the logs). The patch you built tries to replace line 1796 onwards using this:

@@ -1796,6 +1796,10 @@

and my patch tool (this is the merge tool supplied with TortoiseSVN) reports that:

The patch is outdated! The file line 
      // Allow a factor of 2 difference, to cover random variation.
and the patchline

do not match!

If we look at line 1796 of the current revision (11503 is the last one in the logs for queryparsertest.cc) we find a blank line.

I'm not sure what/who's at fault here, to be honest, but I thought it best to clarify.

[Olly Betts]

Thanks for looking in to this.

TortoiseSVN is essentially to blame here - GNU patch doesn't behave that way (it just gives an informational message to say what offset it had to apply to get the patch to match), and that's the de facto standard patch applying tool. One can't expect people to additionally test patches apply with patch programs on an OS they don't run. And indeed a major reason for sending patches rather than the changed sources is that you can still apply them if the underlying code changes in areas not touched by the patch. So not allowing an offset nullifies one big advantage of sending a patch.

You can get a port of GNU patch here:

​http://gnuwin32.sourceforge.net/packages/patch.htm