MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Chuck Bell Date:June 12 2008 3:08pm
Subject:WL#4222 Test patch review
View as plain text  
Hema,

Patch not approved. I have comments.

1) The patch is malformed and includes an extra bit at the top which should
not be there. .bzr-mysql...

2) The tests will not run.

Unexpected line ' ' found in
'd:/source/bzr/mysql-6.0-wl-4222/mysql-test/suite/b
ackup/combinations' at lib//My/Config.pm line 203, <GEN12> line 3.

This was due to extra characters in the file. I changed it to:

--- a/mysql-test/suite/backup/combinations      1970-01-01 00:00:00 +0000 
+++ b/mysql-test/suite/backup/combinations      2008-06-05 18:18:32 +0000 
@@ -0,0 +1,12 @@ 
+[innodb] 
+--default-storage-engine=innodb 
+[falcon] 
+--default-storage-engine=falcon 
+[myisam] 
+--default-storage-engine=myisam 
+[memory] 
+--default-storage-engine=memory 
+

3) Why are these tests in a backup suite but the rest are not? Is there some
other patch that moves the backup tests to the backup suite? Seems silly to
have some in one place and others in another.

4) I don't like the names of some of the tests. The names start out
meaningful but the naming scheme is inconsistent. For example, shouldn't
'backup_objectsfall' be 'backup_functions'? That would seem to make more
sense. When I first saw the test name I though it was a test that is only
run in the Fall...as opposed to Spring, Summer.... :P Same goes for the
other two tests... backup_procedures, backup_triggers.

5) You need to include the 'must have' list to include falcon, innodb in all
of your tests. Not every build will build these engines -- especially
falcon.

6) Why are all of the statements on one line? It would make for easier
reading if, for example, CREATE were written like this:

CREATE TABLE teams (
  teamno int not null primary key,
  playerno int,
  division char(10),
  foreign key(playerno)
  references players(playerno)
);

...you get the idea.

7) It has been requested of me in the past by QA folk that there is a
standard for case usage. For example, all SQL command keywords are uppercase
while identifiers are lowercase. Above should be:

CREATE TABLE teams (
  teamno INT NOT NULL PRIMARY KEY,
  playerno INT,
  division CHAR(10),
  FOREIGN KEY(playerno)
  REFERENCES players(playerno)
);

...please check with Jeb to confirm this.

8) It has also been suggested to me that we should avoid using 'USE db' and
prefix all statements with the db name. SELECT * FROM db.t1; ...please check
with Jeb to confirm this.

9) There are spacing issues. For example:

+ INSERT INTO matches
values(6,1,3,3,1),(7,2,1,4,2),(8,1,5,0,1),(9,2,2,3,0),(10,2,2,2,2) ; 
 ^
^ 

10) Watch line breaks. This should be formatted so that words are broken
across lines at 'legal' points:

+# Purpose: To test the metadata consistency by using object trigger for all
sto 
+# rage engines 

What's a sto rage engine? Is it a really angry sto?

11) Do you really need to mask so many columns?

+--replace_column 2 #  3 # 4 # 5 # 6 # 7 # 8 # 9 # 10 # 11 # 12 # 13 # 14 #
16 # 17 # 18 # 
+SHOW TABLE STATUS; 

If so, then perhaps there is a different command you can use...

12) Once the patch and combinations file was fixed, I got this error:

backup.backup_objectsfall 'innodb' [ fail ]

mysqltest: At line 9: Could not open
'.\data2\bzr6.04june\mysql-6.0-backup\mysql
-test\suite\backup\include\backup_engine.inc' for reading

Which is caused by a hard-coded path in your test file:

--source
/data2/bzr6.04june/mysql-6.0-backup/mysql-test/suite/backup/include/backup_e
ngine.inc 

The above should be:

--source suite/backup/include/backup_engine.inc 

13) Once the patch issue was resolved, the result file was different but
this was due to the patch problem. The same is true for the other two tests.

14) Once all those problems were solved, I got this. I surrender. Please fix
and resubmit a new patch.

backup.backup_objectts 'innodb' [ fail ]

mysqltest: At line 181: query 'RESTORE FROM 'bup_objectts.bak'' failed: 7:
Error
 on rename of '.\bup_objectts\cap.TRG~' to '.\bup_objectts\cap.TRG'
(Errcode: 17
)

The result from queries just before the failure was:
< snip >
Table   t2
Create Table    CREATE TABLE `t2` (
  `a` char(4) DEFAULT NULL
) ENGINE=ENGINE DEFAULT CHARSET=latin1
SHOW TABLE STATUS;
Name    Engine  Version Row_format      Rows    Avg_row_length
Data_lengthMax_d
ata_length      Index_length    Data_free       Auto_increment
Create_timeUpdat
e_time  Check_time      Collation       Checksum        Create_options
Comment
cap     #       #       #       #       #       #       #       #
#####lat
in1_swedish_ci  #       #       #
city    #       #       #       #       #       #       #       #
#####lat
in1_swedish_ci  #       #       #
dropcity        #       #       #       #       #       #       #
######la
tin1_swedish_ci #       #       #
t2      #       #       #       #       #       #       #       #
#####lat
in1_swedish_ci  #       #       #
t3      #       #       #       #       #       #       #       #
#####lat
in1_swedish_ci  #       #       #
t4      #       #       #       #       #       #       #       #
#####lat
in1_swedish_ci  #       #       #
backup data
BACKUP DATABASE bup_objectts TO 'bup_objectts.bak';
backup_id
#
dropping  database.
DROP DATABASE bup_objectts;
RESTORE FROM bup_objectts.bak;
RESTORE FROM 'bup_objectts.bak';

Chuck

Thread
WL#4222 Test patch reviewChuck Bell12 Jun
  • RE: WL#4222 Test patch reviewHema Sridharan13 Jun
    • RE: WL#4222 Test patch reviewChuck Bell13 Jun
      • RE: WL#4222 Test patch reviewHema Sridharan24 Jun
        • RE: WL#4222 Test patch reviewChuck Bell30 Jun
          • RE: WL#4222 Test patch reviewHema Sridharan1 Jul
            • RE: WL#4222 Test patch reviewChuck Bell1 Jul
              • RE: WL#4222 Test patch reviewHema Sridharan2 Jul
                • RE: WL#4222 Test patch reviewChuck Bell2 Jul
                  • RE: WL#4222 Test patch reviewHema Sridharan3 Jul
                    • RE: WL#4222 Test patch reviewChuck Bell21 Jul
                      • RE: WL#4222 Test patch reviewHema Sridharan23 Jul
                        • RE: WL#4222 Test patch reviewChuck Bell23 Jul
                          • Re: WL#4222 Test patch reviewJoerg Bruehe31 Jul
                            • RE: WL#4222 Test patch reviewHema Sridharan1 Aug
                              • Re: WL#4222 Test patch reviewJoerg Bruehe1 Aug
                      • RE: WL#4222 Test patch reviewHema Sridharan7 Aug
                        • RE: WL#4222 Test patch reviewChuck Bell11 Aug
                          • RE: WL#4222 Test patch reviewHema Sridharan11 Aug
RE: WL#4222 Test patch reviewChuck Bell30 Jun
  • RE: WL#4222 Test patch reviewHema Sridharan30 Jun