93667 - (bz-sancheckclean) Sanity check cleanup.

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P3)

Description

[Matthew Tuck [:CodeMachine]]

This is a cleanup to sanitycheck.cgi.  It moves a couple of things around and
adds comments.  There aren't really any code cleanups.

Comment 1

[Matthew Tuck [:CodeMachine]]

Attachment: Cleanup patch.

Comment 2

[Matthew Tuck [:CodeMachine]]

Moving to new Bugzilla product ...

Comment 3

[Matthew Tuck [:CodeMachine]]

Taking ...

Comment 4

[Matthew Tuck [:CodeMachine]]

Comment on attachment 44690 [details] [diff] [review]
Cleanup patch.

This has changed a bit, I'll assume it's out of date.

Comment 5

[Matthew Tuck [:CodeMachine]]

Attachment: Cleaned up sanity check.

Comment 6

[Matthew Tuck [:CodeMachine]]

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 7

[Matthew Tuck [:CodeMachine]]

Comment on attachment 54399 [details]
Cleaned up sanity check.

Whoops, this is the file, not the patch, but the patch is actually bigger anyway.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Matty, can you post the patch as well?

Comment 9

[Matthew Tuck [:CodeMachine]]

Attachment: The good stuff.

Comment 10

[Matthew Tuck [:CodeMachine]]

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.

Comment 11

[Dave Miller [:justdave]]

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.

Comment 12

[Bradley Baetz (:bbaetz)]

Comment on attachment 56459 [details]
The good stuff.

This looks ok, but its badly bitrotted. DoubleCrossCheck needs documentation,
too - are you adding tests?

Comment 13

[Matthew Tuck [:CodeMachine]]

Are you sure?  I pretty much look after sanitycheck.cgi, and I don't know of any
changes there for quite a while.

Comment 14

[Bradley Baetz (:bbaetz)]

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

Comment 15

[Matthew Tuck [:CodeMachine]]

What documentation does DoubleCrossCheck need?  CrossCheck doesn't have any.

Comment 16

[Bradley Baetz (:bbaetz)]

That doesn't mean that it shouldn't be commentless either...

Comment 17

[Gervase Markham [:gerv]]

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

Comment 18

[Bradley Baetz (:bbaetz)]

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.

Comment 19

[Matthew Tuck [:CodeMachine]]

Attachment: Add comments to uncommented sections.

Comment 20

[Matthew Tuck [:CodeMachine]]

Let's take this one bit at a time.

Comment 21

[Bradley Baetz (:bbaetz)]

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.

Comment 22

[Matthew Tuck [:CodeMachine]]

Attachment: Generic double field cross checking facility.

Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.43; previous revision: 1.42
done

Comment 23

[Matthew Tuck [:CodeMachine]]

Attachment: Generic bad bug finding facility.

Comment 24

[Matthew Tuck [:CodeMachine]]

I will add documentation for CrossCheck and DoubleCrossCheck at a later patch,
assuming it is reviewable otherwise.

Comment 25

[Matthew Tuck [:CodeMachine]]

DoubleCrossCheck could be a speedup or a slowdown so we need perf testing there.

Comment 26

[Bradley Baetz (:bbaetz)]

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.

Comment 27

[Bradley Baetz (:bbaetz)]

Comment on attachment 84607 [details] [diff] [review]
Generic bad bug finding facility.

r=bbaetz, ditto

Comment 28

[Matthew Tuck [:CodeMachine]]

> 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.

Comment 29

[Matthew Tuck [:CodeMachine]]

Attachment: CrossCheck rewrite, hopefully improving performance.

Comment 30

[Bradley Baetz (:bbaetz)]

Comment on attachment 94303 [details] [diff] [review]
CrossCheck rewrite, hopefully improving performance.

Looks fine, get myk to test it.

r=bbaetz

Comment 31

[Matthew Tuck [:CodeMachine]]

Attachment: Replacement for previous bitrotten DoubleCrossCheck patch.

Comment 32

[Matthew Tuck [:CodeMachine]]

Attachment: CrossCheck + DoubleCrossCheck + Perf Measuring For Myk (After Case)

Comment 33

[Matthew Tuck [:CodeMachine]]

Attachment: Perl Measuring Only For Myk (Before Case)

Comment 34

[Myk Melez [:myk] [@mykmelez]]

Attachment: After Case Unrotted

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.

Comment 35

[Myk Melez [:myk] [@mykmelez]]

Mattyt- Which DISTINCTs did you want me to test this code without?

Comment 36

[Matthew Tuck [:CodeMachine]]

Attachment: New CrossCheck rewrite, fix minor bug causing slight output error.

Comment 37

[Matthew Tuck [:CodeMachine]]

Attachment: Again unrot DoubleCrossCheck rewrite.

Comment 38

[Matthew Tuck [:CodeMachine]]

Comment on attachment 98536 [details] [diff] [review]
Perl Measuring Only For Myk (Before Case)

Don't need this testing stuff anymore.

Comment 39

[Matthew Tuck [:CodeMachine]]

Comment on attachment 101358 [details] [diff] [review]
After Case Unrotted

Don't need this testing stuff anymore.

Comment 40

[Matthew Tuck [:CodeMachine]]

Comment on attachment 84607 [details] [diff] [review]
Generic bad bug finding facility.

Marking obsolete, checked in.

Comment 41

[Bradley Baetz (:bbaetz)]

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.

Comment 42

[Bradley Baetz (:bbaetz)]

Comment on attachment 101815 [details] [diff] [review]
New CrossCheck rewrite, fix minor bug causing slight output error.

r=bbaetz x2

Comment 43

[Matthew Tuck [:CodeMachine]]

OK, all three patches have now been checked in.  More coming.

Comment 44

[Matthew Tuck [:CodeMachine]]

Comment on attachment 101815 [details] [diff] [review]
New CrossCheck rewrite, fix minor bug causing slight output error.

Marking obsolete, checked in.

Comment 45

[Matthew Tuck [:CodeMachine]]

Comment on attachment 101816 [details] [diff] [review]
Again unrot DoubleCrossCheck rewrite.

Marking obsolete, checked in.

Comment 46

[Matthew Tuck [:CodeMachine]]

Attachment: Move some code around, add some section heading comments.

Comment 47

[Bradley Baetz (:bbaetz)]

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

r=bbaetz x2 on copy-n-paste

Comment 48

[Matthew Tuck [:CodeMachine]]

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

Marking obsolete, checked in.

Comment 49

[Matthew Tuck [:CodeMachine]]

Attachment: More movement, commenting, and remove an unused variable.

Comment 50

[Bradley Baetz (:bbaetz)]

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

r=bbaetz x2

Comment 51

[Matthew Tuck [:CodeMachine]]

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

Marking obsolete, checked in.

Comment 52

[Matthew Tuck [:CodeMachine]]

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

Marking obsolete, checked in.

Comment 53

[Matthew Tuck [:CodeMachine]]

Attachment: Minor style fix, uninit var fix, add explanatory comments to CrossCheck/DoubleCrossCheck.

Comment 54

[Bradley Baetz (:bbaetz)]

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

Comment 55

[Matthew Tuck [:CodeMachine]]

My job here is done, but see also bug #172774 and bug #172775.