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.
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
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
This should be a QA test, not a test in t/.
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
Created attachment 454261 [details] [diff] [review] WIP 4 This correctly tests equals, notequals, substring, and casesubstring.
Attachment #454258 - Attachment is obsolete: true
(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.
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
Created attachment 454308 [details] [diff] [review] WIP 6 Works through lessthan, now.
Attachment #454304 - Attachment is obsolete: true
Created attachment 454317 [details] [diff] [review] WIP 7 This now tests anyexact and contains 8109 tests.
Attachment #454308 - Attachment is obsolete: true
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
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
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
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)
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
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
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
Oh, and 1.5) Make this also test custom fields.
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
Created attachment 455051 [details] [diff] [review] WIP 14 This version now also tests all the custom field types.
Attachment #454840 - Attachment is obsolete: true
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
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
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.
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+
Wooohooo!! Glob, you are my hero! :-) I will update the regex so that it doesn't require English MySQL, on checkin.
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
Added to the release notes in bug 604256.
You need to log in before you can comment on or make changes to this bug.