Closed Bug 93667 (bz-sancheckclean) Opened 23 years ago Closed 22 years ago

Sanity check cleanup.

Categories

(Bugzilla :: Administration, task, P3)

2.13

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: CodeMachine, Assigned: CodeMachine)

Details

Attachments

(1 file, 15 obsolete files)

2.59 KB, patch
bbaetz
: review+
bbaetz
: review+
Details | Diff | Splinter Review
This is a cleanup to sanitycheck.cgi.  It moves a couple of things around and
adds comments.  There aren't really any code cleanups.
Attached patch Cleanup patch. (obsolete) — Splinter Review
Keywords: patch, review
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.16
Moving to new Bugzilla product ...
Component: Bugzilla → Administration
Product: Webtools → Bugzilla
Version: Bugzilla 2.13 → 2.13
Taking ...
Assignee: justdave → matty
QA Contact: matty → jake
Status: NEW → ASSIGNED
Comment on attachment 44690 [details] [diff] [review]
Cleanup patch.

This has changed a bit, I'll assume it's out of date.
Attachment #44690 - Attachment is obsolete: true
Keywords: patch, review
Attached file Cleaned up sanity check. (obsolete) —
This patch does the following:

1.  Fixes up an uninit error on $lastb.
2.  Introduces extra comments.
3.  Moves stuff around to better places.
4.  Adds BugStatus and DoubleCrossCheck that result in removing about 80 lines
from the CGI.
5.  Changes the implementation of CrossCheck to match DoubleCrossCheck.

I'm not sure whether the new implementation of cross checks would be faster, but
I suspect so as it is only one query.

I'd like it to be put on bmo as sanitycheck2.cgi at some stage to test perf and
make sure the same errors are found.  Since this is sanity checking there is no
danger.
Comment on attachment 54399 [details]
Cleaned up sanity check.

Whoops, this is the file, not the patch, but the patch is actually bigger anyway.
Attachment #54399 - Attachment is patch: false
Keywords: patch, review
Matty, can you post the patch as well?
Attachment #54399 - Attachment is obsolete: true
Attached file The good stuff. (obsolete) —
The above is a .tar.bz2 which contains the previous patch (with a few tweaks),
split into 10 stages that get applied in order.  This should improve the
reviewability of the patch.
We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Comment on attachment 56459 [details]
The good stuff.

This looks ok, but its badly bitrotted. DoubleCrossCheck needs documentation,
too - are you adding tests?
Attachment #56459 - Flags: review-
Are you sure?  I pretty much look after sanitycheck.cgi, and I don't know of any
changes there for quite a while.
Its at least as old as the sanity check stuff - your patch adds that, but so did
I, earlier. Extra tests have been added, and so on. They're only a few lines
here and there, but you've moved so much code that the entire thing was just a
huge conflict
What documentation does DoubleCrossCheck need?  CrossCheck doesn't have any.
That doesn't mean that it shouldn't be commentless either...
Is this patch going to be refreshed or revived for 2.16? If not, we need to work
out what to do over in bug 104589.

> The above is a .tar.bz2 which contains the previous patch (with a few tweaks),
> split into 10 stages that get applied in order.  This should improve the
> reviewability of the patch.

Anything which means patches are not attached as text/plain text files decreases
their reviewability by orders of magnitude. As does making you apply ten files
in the right order instead of one.

Gerv

No, this was a lot easier to review in bits, since the changes are separate. The
only problem I had with it was that it was bitrotted.
Attachment #56459 - Attachment is obsolete: true
Let's take this one bit at a time.
Comment on attachment 84598 [details] [diff] [review]
Add comments to uncommented sections.

r=bbaetz x2

I was happy to review the lots-of-little-patches thing, btw. You should at
least combine the stuff which just moves code without changing it.
Attachment #84598 - Flags: review+
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.43; previous revision: 1.42
done
Attachment #84598 - Attachment is obsolete: true
I will add documentation for CrossCheck and DoubleCrossCheck at a later patch,
assuming it is reviewable otherwise.
DoubleCrossCheck could be a speedup or a slowdown so we need perf testing there.
Alias: bz-sancheckclean
Comment on attachment 84606 [details] [diff] [review]
Generic double field cross checking facility.

r=bbaetz

This looks fine, visually. When myk runs timing tests on the bmo database, he
should check the results are teh same.

You need docs, too.
Attachment #84606 - Flags: review+
Comment on attachment 84607 [details] [diff] [review]
Generic bad bug finding facility.

r=bbaetz, ditto
Attachment #84607 - Flags: review+
> When myk runs timing tests on the bmo database, he should
> check the results are teh same.

Well, they should be *better* for DoubleCrossCheck, but you never know with the
MySQL optimiser.  The bad bug stuff has no need to be time tested.

I will try to remake the CrossCheck patch so we can test that at the same time,
I can't see one being a speedup/slowdown and the other not the same.
Attachment #84606 - Flags: review+
Attachment #84607 - Flags: review+
Comment on attachment 94303 [details] [diff] [review]
CrossCheck rewrite, hopefully improving performance.

Looks fine, get myk to test it.

r=bbaetz
Attachment #94303 - Flags: review+
Attachment #84606 - Attachment is obsolete: true
Attached patch After Case Unrotted (obsolete) — Splinter Review
The after case patch has rotted somewhat.  Here's an unrotted version.	I'm not
an expert in this code, so I may have missed something.
Attachment #98534 - Attachment is obsolete: true
Mattyt- Which DISTINCTs did you want me to test this code without?
Attachment #98533 - Attachment is obsolete: true
Comment on attachment 98536 [details] [diff] [review]
Perl Measuring Only For Myk (Before Case)

Don't need this testing stuff anymore.
Attachment #98536 - Attachment is obsolete: true
Comment on attachment 101358 [details] [diff] [review]
After Case Unrotted

Don't need this testing stuff anymore.
Attachment #101358 - Attachment is obsolete: true
Comment on attachment 84607 [details] [diff] [review]
Generic bad bug finding facility.

Marking obsolete, checked in.
Attachment #84607 - Attachment is obsolete: true
Comment on attachment 101816 [details] [diff] [review]
Again unrot DoubleCrossCheck rewrite.

r=bbaetz x2. Although I don't have a db with these sort of errors to test with,
it looks sensible, and it found the same issues on bmo.
Attachment #101816 - Flags: review+
Comment on attachment 101815 [details] [diff] [review]
New CrossCheck rewrite, fix minor bug causing slight output error.

r=bbaetz x2
Attachment #101815 - Flags: review+
OK, all three patches have now been checked in.  More coming.
Comment on attachment 101815 [details] [diff] [review]
New CrossCheck rewrite, fix minor bug causing slight output error.

Marking obsolete, checked in.
Attachment #101815 - Attachment is obsolete: true
Attachment #101816 - Attachment is obsolete: true
Comment on attachment 101816 [details] [diff] [review]
Again unrot DoubleCrossCheck rewrite.

Marking obsolete, checked in.
Comment on attachment 101817 [details] [diff] [review]
Move some code around, add some section heading comments.

r=bbaetz x2 on copy-n-paste
Attachment #101817 - Flags: review+
Comment on attachment 101817 [details] [diff] [review]
Move some code around, add some section heading comments.

Marking obsolete, checked in.
Comment on attachment 101820 [details] [diff] [review]
More movement, commenting, and remove an unused variable.

r=bbaetz x2
Attachment #101820 - Flags: review+
Comment on attachment 101817 [details] [diff] [review]
Move some code around, add some section heading comments.

Marking obsolete, checked in.
Attachment #101817 - Attachment is obsolete: true
Comment on attachment 101820 [details] [diff] [review]
More movement, commenting, and remove an unused variable.

Marking obsolete, checked in.
Attachment #101820 - Attachment is obsolete: true
Comment on attachment 101822 [details] [diff] [review]
Minor style fix, uninit var fix, add explanatory comments to CrossCheck/DoubleCrossCheck.

r=bbaetz x2 if you use != instead of ne for the numeric bug numbers
Attachment #101822 - Flags: review+
My job here is done, but see also bug #172774 and bug #172775.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: jake → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: