Bug 574879 (bz-search-test)

Create a test that assures the correctness of Search.pm's boolean charts

RESOLVED FIXED in Bugzilla 4.0

Status

()

Bugzilla
Testing Suite
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

Bugzilla 4.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 17 obsolete attachments)

v1
117.27 KB, patch
glob
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
I'm going to write a test that tests every combination of operators and fields in Search.pm. It will modify the database, so it won't run unless a special BZ_WRITE_TESTS environment variable is set.
(Assignee)

Comment 1

8 years ago
Created attachment 454244 [details] [diff] [review]
Work In Progress

Okay, here's a script that tests *all* of the fields except flagtypes.name, using the "equals" operator.
Assignee: testing → mkanat
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 years ago
Created attachment 454254 [details] [diff] [review]
WIP 2

Here's another Work In Progress--this one implements notequals, and exposes a ton of bugs in Search.pm.
Attachment #454244 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Depends on: 574940

Comment 3

8 years ago
This should be a QA test, not a test in t/.
(Assignee)

Comment 4

8 years ago
Created attachment 454258 [details] [diff] [review]
WIP 3

Okay, due to a slight error in WIP #1, this version is actually the first version that tests *all* the fields with "equals".
Attachment #454254 - Attachment is obsolete: true
(Assignee)

Comment 5

8 years ago
Created attachment 454261 [details] [diff] [review]
WIP 4

This correctly tests equals, notequals, substring, and casesubstring.
Attachment #454258 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
(In reply to comment #3)
> This should be a QA test, not a test in t/.

  Well, I thought about that, but I'd rather have it in t/. I want it to run on trunk all the time. Also, it needs its own whole setup, which it does itself, and doesn't run unless you set a special environment variable. Also, it's just a code test, not a functional test of the UI or the API.
(Assignee)

Comment 7

8 years ago
Created attachment 454304 [details] [diff] [review]
WIP 5

This Work In Progress goes up to testing regexes. Also, I now test all the flag fields.
Attachment #454261 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
Created attachment 454308 [details] [diff] [review]
WIP 6

Works through lessthan, now.
Attachment #454304 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
Created attachment 454317 [details] [diff] [review]
WIP 7

This now tests anyexact and contains 8109 tests.
Attachment #454308 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Blocks: 574556
No longer depends on: 574556
(Assignee)

Comment 10

8 years ago
Created attachment 454413 [details] [diff] [review]
WIP 8

This tests through nowordssubstr. Also, I refactored the test so that it should never say that any test UNEXPECTEDLY SUCCEEDED anymore. That way we can know if there's something that we have to update about the test itself, and it also lets us be much more specific, programatically, about exactly what is broken.
Attachment #454317 - Attachment is obsolete: true
(Assignee)

Comment 11

8 years ago
Created attachment 454441 [details] [diff] [review]
WIP 9

Okay, this searches every operator type except for the "changed" types (which will probably be some fair bit of additional work to test).
Attachment #454413 - Attachment is obsolete: true
(Assignee)

Comment 12

8 years ago
Created attachment 454456 [details] [diff] [review]
WIP 10 (Backportable to 3.6)

Okay, this version now creates two sets of values--one for bug creation, and another to update all the field values, so that we can have values in bugs_activity for the changed* search types.
Attachment #454441 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
Comment on attachment 454456 [details] [diff] [review]
WIP 10 (Backportable to 3.6)

This is the last version that will be backportable to 3.6--later versions use $bug->set_all and so require 4.0.
Attachment #454456 - Attachment description: WIP 10 → WIP 10 (Backportable to 3.6)
(Assignee)

Comment 14

8 years ago
Created attachment 454500 [details] [diff] [review]
WIP 11

Yet another work in progress. This one tests changedbefore, changedafter, and changedto. changedby should be pretty easy; changedfrom will be tricky.
Attachment #454456 - Attachment is obsolete: true
(Assignee)

Comment 15

8 years ago
Created attachment 454754 [details] [diff] [review]
WIP 12

Okay, this crazy complex thing now also can test changedfrom--that was probably the biggest complexity-adder I'm going to have to do. Now just changedby and I'll have one that tests all the operators!
Attachment #454500 - Attachment is obsolete: true
(Assignee)

Comment 16

8 years ago
Created attachment 454757 [details] [diff] [review]
WIP 12, All Operators Tested

Okay, this now tests every operator combined with every field! More or less, this could now be ready for review, except that I want to:

1) Refactor this into objects that run tests, because the code is getting
   way too complex.
2) Add AND and OR tests.
3) Possibly add SQL injection tests.
4) Possibly add multiple-chart tests.
Attachment #454754 - Attachment is obsolete: true
(Assignee)

Comment 17

8 years ago
Oh, and 1.5) Make this also test custom fields.
(Assignee)

Comment 18

8 years ago
Created attachment 454840 [details] [diff] [review]
WIP 13

All right, this one is now a series of objects. It's much, much cleaner--it could possibly even be understood by somebody other than me! :-) There are also a LOT of comments now, explaining each bit, particularly for the constants.
Attachment #454757 - Attachment is obsolete: true
(Assignee)

Comment 19

8 years ago
Created attachment 455051 [details] [diff] [review]
WIP 14

This version now also tests all the custom field types.
Attachment #454840 - Attachment is obsolete: true
(Assignee)

Comment 20

8 years ago
Created attachment 455332 [details] [diff] [review]
WIP 15

This version now tests for all the sorts of SQL injections I could think of. It works by trying to insert SQL that would cause the database to throw an error or fail in some way if there was an injection, and then making sure that no error is thrown.
Attachment #455051 - Attachment is obsolete: true
(Assignee)

Comment 21

8 years ago
Created attachment 455373 [details] [diff] [review]
WIP 16

Okay, this now does OR tests with every field/operator combination, making it about 30 million tests now. I haven't finished a test run yet, but it's already exposing various situations in which OR charts fail. (For example, HAVING breaks OR charts, because it limits the entire query.)
Attachment #455332 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Alias: bz-search-test
(Assignee)

Updated

8 years ago
Depends on: 576670
(Assignee)

Updated

8 years ago
Blocks: 576670
No longer depends on: 576670
(Assignee)

Comment 22

8 years ago
Created attachment 455974 [details] [diff] [review]
v1

Okay, here it is! This tests:

* Every field/operator combination. It doesn't test days_elapsed and owner_idle_time, because those only currently work with a very small set of operators, and are easy enough to test manually (and would have been a lot of work to add to the script). It also doesn't test attachments.isurl, because that would have been a lot of effort for no gain (since we already test the other three attachment boolean fields with identical semantics).

* For each field/operator combination, it knows which ones are broken, currently. If one of those starts working, the test will say "unexpectedly succeeded" so that we can modify the test.

* Attempts to do a SQL injection via every field/operator combination (even days_elapsed and owner_idle_time). We know if the SQL injection succeeded if running the SQL throws an error (because we inject things that would be invalid SQL if the injection succeeded).

* Optionally, every possible AND/OR combination of every field/operator combination. To enable this, pass "--long" to the test, like:

  BZ_WRITE_TESTS=1 prove xt/ :: --long

However, you should be aware that the --long test probably can't ever complete, if you don't limit it somehow. To do a smaller --long test, you can use the --top-operators switch to the test, described in its help. (See xt/README.) However, until bug 576670 (the "speed up Search::init" bug) is complete, the test is really too slow to run --long anyhow. --long won't run in the tinderboxen for 4.0, it will only run for 4.2.

You should also be aware that there are thousands of failing tests under --long.

I don't do much tracking of known-broken combinations for AND/OR tests, because that would have taken me several weeks to document for what I felt was little gain.

* If you pass --add-custom-fields to the test, it will also add one of every type of custom field to the database, so that they can also be tested.


This patch is roughly the size of a small application. It's larger than Search.pm itself. I don't expect a detailed code review, but I think that since it's a test, a quick test from you and a quick review of the code would be appreciated.
Attachment #455373 - Attachment is obsolete: true
Attachment #455974 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Target Milestone: --- → Bugzilla 4.0
(Assignee)

Comment 23

8 years ago
Oh, also, you'll notice that I added a new usage_mode, USAGE_MODE_TEST. That's to prevent things from spitting out messages that normally spit out messages if we're on the command line (like Install::make_admin, Field->create, etc.) and also to enable ERROR_MODE_TEST (which is mostly useful for performance, because Throw*Error is slow otherwise, and there's no good reason to speed it up for normal Bugzilla usage).
Comment on attachment 455974 [details] [diff] [review]
v1

r=glob by inspection

either the documentation or code need to be updated to indicate that the tests currently require english mysql.
Attachment #455974 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 25

8 years ago
Wooohooo!! Glob, you are my hero! :-)

I will update the regex so that it doesn't require English MySQL, on checkin.
(Assignee)

Updated

8 years ago
Flags: approval+
(Assignee)

Comment 26

8 years ago
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla.pm
added xt
modified Bugzilla/Bug.pm
modified Bugzilla/Constants.pm
modified Bugzilla/Error.pm
modified Bugzilla/Install.pm
modified Bugzilla/Search.pm
modified Bugzilla/Install/Filesystem.pm
added xt/README
added xt/lib
added xt/search.t
added xt/lib/Bugzilla
added xt/lib/Bugzilla/Test
added xt/lib/Bugzilla/Test/Search
added xt/lib/Bugzilla/Test/Search.pm
added xt/lib/Bugzilla/Test/Search/AndTest.pm
added xt/lib/Bugzilla/Test/Search/Constants.pm
added xt/lib/Bugzilla/Test/Search/FakeCGI.pm
added xt/lib/Bugzilla/Test/Search/FieldTest.pm
added xt/lib/Bugzilla/Test/Search/InjectionTest.pm
added xt/lib/Bugzilla/Test/Search/OperatorTest.pm
added xt/lib/Bugzilla/Test/Search/OrTest.pm
Committed revision 7288.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Blocks: 577388
(Assignee)

Updated

8 years ago
Keywords: relnote
(Assignee)

Comment 27

8 years ago
Added to the release notes in bug 604256.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.