Bug 657542: Make the AND/OR tests for xt/search.t function properly and

catch known-broken tests as it's supposed to.
r=mkanat, a=mkanat (module owner)

6 files changed, 64 insertions, 94 deletions

diff --git a/xt/lib/Bugzilla/Test/Search.pm b/xt/lib/Bugzilla/Test/Search.pm
index 133996283..f18d3e4ca 100644
--- a/xt/lib/Bugzilla/Test/Search.pm
+++ b/xt/lib/Bugzilla/Test/Search.pm
@@ -851,6 +851,20 @@ sub value_translation_cache {
     return $self->{value_translation_cache}->{$test_name};
 }
 
+# When doing AND/OR tests, the value for transformed_value_was_equal
+# (see Bugzilla::Test::Search::FieldTest) won't be recalculated
+# if we pull our values from the value_translation_cache. So we need
+# to also cache the values for transformed_value_was_equal.
+sub was_equal_cache {
+    my ($self, $field_test, $number, $value) = @_;
+    return if !$self->option('long');
+    my $test_name = $field_test->name;
+    if (@_ == 4) {
+        $self->{tvwe_cache}->{$test_name}->{$number} = $value;
+    }
+    return $self->{tvwe_cache}->{$test_name}->{$number};
+}
+
 #############
 # Main Test #
 #############
diff --git a/xt/lib/Bugzilla/Test/Search/AndTest.pm b/xt/lib/Bugzilla/Test/Search/AndTest.pm
index b4554584b..b7f8b3cab 100644
--- a/xt/lib/Bugzilla/Test/Search/AndTest.pm
+++ b/xt/lib/Bugzilla/Test/Search/AndTest.pm
@@ -40,12 +40,10 @@ sub bug_is_contained {
     return all { $_->bug_is_contained($number) } $self->field_tests;
 }
 
-########################
-# SKIP & TODO Messages #
-########################
-
-sub _join_skip { () }
-sub _join_broken_constant { {} }
+sub _bug_will_actually_be_contained {
+    my ($self, $number) = @_;
+    return all { $_->will_actually_contain_bug($number) } $self->field_tests;
+}
 
 ##############################
 # Bugzilla::Search arguments #
@@ -65,4 +63,4 @@ sub search_params {
     return \%params;
 }
 
-1;
\ No newline at end of file
+1;
diff --git a/xt/lib/Bugzilla/Test/Search/Constants.pm b/xt/lib/Bugzilla/Test/Search/Constants.pm
index a06ceecc7..512d180d9 100644
--- a/xt/lib/Bugzilla/Test/Search/Constants.pm
+++ b/xt/lib/Bugzilla/Test/Search/Constants.pm
@@ -46,7 +46,6 @@ our @EXPORT = qw(
     KNOWN_BROKEN
     NUM_BUGS
     NUM_SEARCH_TESTS
-    OR_BROKEN
     SKIP_FIELDS
     SPECIAL_PARAM_TESTS
     SUBSTR_NO_FIELD_ADD
@@ -299,10 +298,10 @@ use constant KNOWN_BROKEN => {
         CHANGED_VALUE_BROKEN,
         # All fields should have a way to search for "changing
         # from a blank value" probably.
-        blocked   => { contains => [3,4,5] },
-        dependson => { contains => [2,4,5] },
+        blocked   => { contains => [3,4,5], no_criteria => 1 },
+        dependson => { contains => [2,4,5], no_criteria => 1 },
         work_time => { contains => [1] },
-        FIELD_TYPE_BUG_ID, { contains => [5] },
+        FIELD_TYPE_BUG_ID, { contains => [5], no_criteria => 1 },
     },
     # changeto doesn't find remaining_time changes (possibly due to us not
     # tracking that data properly).
@@ -941,39 +940,6 @@ use constant INJECTION_TESTS => (
     { value => '/*STAR_COMMENT_TEST' }
 );
 
-# This overrides KNOWN_BROKEN for OR configurations.
-# It indicates that these combinations are broken in some way that they
-# aren't broken when alone, because they don't return what they logically
-# should when put into an OR.
-use constant OR_BROKEN => {
-    # Multi-value fields search on individual values, so "equals" OR "notequals"
-    # returns nothing, when it should instead logically return everything.
-    'blocked-equals' => {
-        'blocked-notequals' => { contains => [1,2,3,4,5] },
-    },
-    'dependson-equals' => {
-        'dependson-notequals' => { contains => [1,2,3,4,5] },
-    },
-    'bug_group-equals' => {
-        'bug_group-notequals' => { contains => [1,2,3,4,5] },
-    },
-    'cc-equals' => {
-        'cc-notequals' => { contains => [1,2,3,4,5] },
-    },
-    'commenter-equals' => {
-        'commenter-notequals' => { contains => [1,2,3,4,5] },
-        'longdesc-notequals'  => { contains => [2,3,4,5] },
-        'longdescs.isprivate-notequals' => { contains => [2,3,4,5] },
-        'work_time-notequals' => { contains => [2,3,4,5] },
-    },
-    'commenter-notequals' => {
-        'commenter-equals' => { contains => [1,2,3,4,5] },
-        'longdesc-equals'  => { contains => [1] },
-        'longdescs.isprivate-equals' => { contains => [1] },
-        'work_time-equals' => { contains => [1] },
-    },
-};
-
 #################
 # Special Tests #
 #################
diff --git a/xt/lib/Bugzilla/Test/Search/CustomTest.pm b/xt/lib/Bugzilla/Test/Search/CustomTest.pm
index d19a9c350..62a622594 100644
--- a/xt/lib/Bugzilla/Test/Search/CustomTest.pm
+++ b/xt/lib/Bugzilla/Test/Search/CustomTest.pm
@@ -53,6 +53,8 @@ sub operator_test { die "unimplemented" }
 sub field_object { die "unimplemented" }
 sub main_value { die "unimplenmented" }
 sub test_value { die "unimplemented" }
+# Custom tests don't use transforms.
+sub transformed_value_was_equal { 0 }
 sub debug_value {
     my ($self) = @_;
     my $string = '';
diff --git a/xt/lib/Bugzilla/Test/Search/FieldTest.pm b/xt/lib/Bugzilla/Test/Search/FieldTest.pm
index 02e0df06c..283a90d16 100644
--- a/xt/lib/Bugzilla/Test/Search/FieldTest.pm
+++ b/xt/lib/Bugzilla/Test/Search/FieldTest.pm
@@ -156,9 +156,12 @@ sub debug_value {
 # result was equal to its first value.
 sub transformed_value_was_equal {
     my ($self, $number, $value) = @_;
-    if (defined $value) {
+    if (@_ > 2) {
         $self->{transformed_value_was_equal}->{$number} = $value;
+        $self->search_test->was_equal_cache($self, $number, $value);
     }
+    my $cached = $self->search_test->was_equal_cache($self, $number);
+    return $cached if defined $cached;
     return $self->{transformed_value_was_equal}->{$number};
 }
 
@@ -215,6 +218,23 @@ sub contains_known_broken {
     return undef;
 }
 
+# Used by subclasses. Checks both bug_is_contained and contains_known_broken
+# to tell you whether or not the bug will *actually* be found by the test.
+sub will_actually_contain_bug {
+    my ($self, $number) = @_;
+    my $is_contained = $self->bug_is_contained($number) ? 1 : 0;
+    my $is_broken = $self->contains_known_broken($number) ? 1 : 0;
+
+    # If the test is supposed to contain the bug and *isn't* broken,
+    # then the test will contain the bug.
+    return 1 if ($is_contained and !$is_broken);
+    # If this test is *not* supposed to contain the bug, but that test is
+    # broken, then this test *will* contain the bug.
+    return 1 if (!$is_contained and $is_broken);
+
+    return 0;
+}
+
 # Returns a string if creating a Bugzilla::Search object throws an error,
 # with this field/operator/value combination.
 sub search_known_broken {
diff --git a/xt/lib/Bugzilla/Test/Search/OrTest.pm b/xt/lib/Bugzilla/Test/Search/OrTest.pm
index 42dc30698..b1dbf646c 100644
--- a/xt/lib/Bugzilla/Test/Search/OrTest.pm
+++ b/xt/lib/Bugzilla/Test/Search/OrTest.pm
@@ -25,7 +25,7 @@ package Bugzilla::Test::Search::OrTest;
 use base qw(Bugzilla::Test::Search::FieldTest);
 
 use Bugzilla::Test::Search::Constants;
-use List::MoreUtils qw(any uniq);
+use List::MoreUtils qw(all any uniq);
 
 use constant type => 'OR';
 
@@ -70,16 +70,8 @@ sub debug_value {
 # SKIP & TODO Messages #
 ########################
 
-sub _join_skip { () }
-sub _join_broken_constant { OR_BROKEN }
-
 sub field_not_yet_implemented {
     my ($self) = @_;
-    foreach my $test ($self->field_tests) {
-        if (grep { $_ eq $test->field } $self->_join_skip) {
-            return $test->field . " is not yet supported in OR tests";
-        }
-    }
     return $self->_join_messages('field_not_yet_implemented');
 }
 sub invalid_field_operator_combination {
@@ -100,17 +92,13 @@ sub _join_messages {
 
 sub _bug_will_actually_be_contained {
     my ($self, $number) = @_;
-    my @results;
+
     foreach my $test ($self->field_tests) {
-        if ($test->bug_is_contained($number)
-            and !$test->contains_known_broken($number))
-        {
-            return 1;
-        }
-        elsif (!$test->bug_is_contained($number)
-               and $test->contains_known_broken($number)) {
-            return 1;
-        }
+        # Some tests are broken in such a way that they actually
+        # generate no criteria in the SQL. In this case, the only way
+        # the test contains the bug is if *another* test contains it.
+        next if $test->_known_broken->{no_criteria};
+        return 1 if $test->will_actually_contain_bug($number);
     }
     return 0;
 }
@@ -118,46 +106,28 @@ sub _bug_will_actually_be_contained {
 sub contains_known_broken {
     my ($self, $number) = @_;
 
-    my $join_broken = $self->_join_known_broken;
-    if (my $contains = $join_broken->{contains}) {
-        my $contains_is_broken = grep { $_ == $number } @$contains;
-        if ($contains_is_broken) {
-            my $name = $self->name;
-            return "$name contains $number is broken";
-        }
-        return undef;
-    }
-
-    return $self->_join_contains_known_broken($number);
-}
-
-sub _join_contains_known_broken {
-    my ($self, $number) = @_;
-    
     if ( ( $self->bug_is_contained($number)
            and !$self->_bug_will_actually_be_contained($number) )
         or ( !$self->bug_is_contained($number)
              and $self->_bug_will_actually_be_contained($number) ) )
     {
-        my @messages = map { $_->contains_known_broken($number) } $self->field_tests;
+        my @messages = map { $_->contains_known_broken($number) } 
+                           $self->field_tests;
         @messages = grep { $_ } @messages;
+        # Sometimes, with things that break because of no_criteria, there won't
+        # be anything in @messages even though we need to print out a message.
+        if (!@messages) {
+            my @no_criteria = grep { $_->_known_broken->{no_criteria} }
+                                   $self->field_tests;
+            @messages = map { "No criteria generated by " . $_->name }
+                            @no_criteria;
+        }
+        die "broken test with no message" if !@messages;
         return join(' AND ', @messages);
     }
     return undef;
 }
 
-sub _join_known_broken {
-    my ($self) = @_;
-    my $or_broken = $self->_join_broken_constant;
-    foreach my $test ($self->field_tests) {
-        @or_broken_for = map { $_->join_broken($or_broken) } $self->field_tests;
-        @or_broken_for = grep { defined $_ } @or_broken_for;
-        last if !@or_broken_for;
-        $or_broken = $or_broken_for[0];
-    }
-    return $or_broken;
-}
-
 ##############################
 # Bugzilla::Search arguments #
 ##############################
@@ -182,4 +152,4 @@ sub search_params {
     return \%params;
 }
 
-1;
\ No newline at end of file
+1;