Bug 12647 - QueryParser fails tests
Summary: QueryParser fails tests
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Galen Charlton
URL:
Keywords:
: 12182 (view as bug list)
Depends on:
Blocks: 12742
  Show dependency treegraph
 
Reported: 2014-07-24 09:14 UTC by Colin Campbell
Modified: 2020-02-24 20:53 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12647: PQF QueryParser driver and unit tests fixes (7.47 KB, patch)
2014-08-08 18:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 12647: PQF QueryParser driver and unit tests fixes (7.52 KB, patch)
2014-08-10 21:10 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 12647: PQF QueryParser driver and unit tests fixes (7.62 KB, patch)
2014-08-10 21:31 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2014-07-24 09:14:30 UTC
QueryParser.t will usually fail using a more current perl.

Here are two runs of QueryParser.t  using perl 5.18.1

~/kohaclone$ prove t/QueryParser.t 
t/QueryParser.t .. 1/? 
#   Failed test 'super simple keyword query'
#   at t/QueryParser.t line 22.
#          got: '@attr 1=1003 @attr 4=6 "smith"'
#     expected: '@or @or @attr 1=1016 @attr 4=6 "smith" @attr 9=20 @attr 2=102 @attr 4=6 "smith" @attr 9=34 @attr 2=102 @attr 4=6 "smith"'

#   Failed test 'keyword search sorted by acqdate descending'
#   at t/QueryParser.t line 33.
#          got: '@or @attr 7=1 @attr 1=32 0 @attr 1=1003 @attr 4=6 "smith"'
#     expected: '@or @attr 1=32 @attr 7=1 0 @attr 1=1003 @attr 4=6 "smith"'
# Looks like you failed 2 tests of 28.
t/QueryParser.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/28 subtests 

Test Summary Report
-------------------
t/QueryParser.t (Wstat: 512 Tests: 28 Failed: 2)
  Failed tests:  6, 17
  Non-zero exit status: 2
Files=1, Tests=28,  0 wallclock secs ( 0.02 usr  0.01 sys +  0.17 cusr  0.01 csys =  0.21 CPU)
Result: FAIL
~/kohaclone$ prove t/QueryParser.t 
t/QueryParser.t .. 1/? 
#   Failed test 'super simple keyword query'
#   at t/QueryParser.t line 22.
#          got: '@attr 1=4 @attr 4=6 "smith"'
#     expected: '@or @or @attr 1=1016 @attr 4=6 "smith" @attr 9=20 @attr 2=102 @attr 4=6 "smith" @attr 9=34 @attr 2=102 @attr 4=6 "smith"'

#   Failed test 'relevance-bumped query'
#   at t/QueryParser.t line 26.
#          got: '@attr 1=4 @attr 2=102 @attr 9=20 @attr 4=6 "smith"'
#     expected: '@attr 1=4 @attr 9=20 @attr 2=102 @attr 4=6 "smith"'

#   Failed test 'keyword search sorted by acqdate descending'
#   at t/QueryParser.t line 33.
#          got: '@or @attr 7=1 @attr 1=32 0 @attr 1=1003 @attr 4=6 "smith"'
#     expected: '@or @attr 1=32 @attr 7=1 0 @attr 1=1003 @attr 4=6 "smith"'
# Looks like you failed 3 tests of 28.
t/QueryParser.t .. Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/28 subtests 

Test Summary Report
-------------------
t/QueryParser.t (Wstat: 768 Tests: 28 Failed: 3)
  Failed tests:  6, 10, 17
  Non-zero exit status: 3
Files=1, Tests=28,  0 wallclock secs ( 0.03 usr  0.01 sys +  0.18 cusr  0.01 csys =  0.23 CPU)
Result: FAIL

The nature of the fails suggests that the parser is assuming that elements in a hash are ordered and generating the query from that. While that has never been the case current versions of perl ensure that hash elements are not returned in the same sequence as a predictable one was a security weakness. So if running with a later perl the dequence of elements in the generated query is random
Comment 1 Tomás Cohen Arazi 2014-08-08 18:00:20 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2014-08-10 13:05:09 UTC
*** Bug 12182 has been marked as a duplicate of this bug. ***
Comment 3 Chris Cormack 2014-08-10 21:10:06 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2014-08-10 21:31:48 UTC
Created attachment 30671 [details] [review]
[PASSED QA] Bug 12647: PQF QueryParser driver and unit tests fixes

Due to Perl 5.18, QueryParser the default search class is no longer
'keyword' (see bug 12738), and needs to be set manually. This patch
adds a line that does that. The problem that gets fixed is with test
'super simple keyword query'.

The rest of the non-deterministically failing tests are due to the same
problem, keys returning differently sorted keys from hashes.

So this patch sorts keys in the step that concatenates attributes when building
the PQF queries (and tests get adjusted to match the now deterministic result).

I did that (sorting there) under Jared's recommendation. Hopefuly he will step
in and comment/fix any mistake I made. My main concern was a possible loss
in performance. That we agreed it is almost void, because of the tiny size
of the hash.

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests are passing now again :)
Comment 5 Tomás Cohen Arazi 2014-08-11 13:25:50 UTC
Patch pushed to master.
Comment 6 Mason James 2015-07-25 08:26:45 UTC
Pushed to 3.16.x, will be in 3.16.13
Comment 7 Mason James 2015-07-25 08:31:24 UTC
(In reply to Mason James from comment #6)
> Pushed to 3.16.x, will be in 3.16.13

A recent VM upgrade to perl-5.20/deb8 seems to have caused this test to fail, for 3.16.x