MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Hema Sridharan Date:June 13 2008 3:00am
Subject:RE: WL#4222 Test patch review
View as plain text  
Hi Chuck,

Thanks for all your comments. At this juncture, I want to update you few
things. Lars, I request you to pitch in and add more clarity or if I'm
missing out any details here.

1. It was decided sometime ago that we will have only BACKUP SUITE and Lars
is aware of that. Also regarding the usage of combinations file, the history
goes like this. (I started with hard coded engines in the script -> Lars
suggested .opt file -> Other folks pointed out the hitches using it -> Moved
to combinations file approach [based on feedback from Lars, Magnus]). 

2. Magnus had sent an email (copied you also) indicating that all test cases
will be moved to backup suite and thats the reason I created this suite and
added test case. I will also add the test cases for other backup related WL
in this suite only.

3. I kept resolving issues, one after other for pushing my tests (commit
issues, .opt, combination file, bzr migration)into the suite, though I had
them ready and I had them tested before manually. Still I agree that there
was little bit of delay and I shall diligently work on this and get the test
cases into suite asap.

Please read in line for my replies and let me know, if you are fine. Based
on your final approval I will push the test suite.

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

Hema: Agreed, will make necessary modifications.


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

Hema: Ok.

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

Hema: Explained above.

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

Hema: Agreed. Will change it :-)

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

Hema: Will inlcude have_falcon.inc in all my tests.

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

Hema : Agreed. Will change it.


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

Hema: I have already shown the same script to Jeb, he did not give me any
specific review comments on SQL command keywords and identifiers. Anyways I
have changed it in accordance to your suggestion :-)



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

Hema: I was feeling comfortable to use "USE db". But if thats not the
suggested option, I can change it.


> 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) ;  ^ ^ 
> 

Hema: Agreed, will make necessary modifications.


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

Hema: :-)! Oops missed the line breaks, will modify that.

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

Hema: I wanted to maintain a uniqueness in result file for all engines. I
realise that this command isn't required and will remove it from the script.

> 
> 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/in
> clude/backup_e
> ngine.inc 
> 
> The above should be:
> 
> --source suite/backup/include/backup_engine.inc 

Hema: Sorry for hardcoding the path name. I have changed it.


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


Hema: Oops ! I didn't get this error when I run it in my machine and thats
why I committed the test. I don't know why it is failing in your machine,
please help me out to figure out the delta b/w these 2 systems.Also this
error is pertinent to "ALTER TABLE" operation, which I haven't done in my
test case. I don't know from where this error pops up?


> 
> 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';
> 

Hema: Once again, I don't see this error and the test runs fine in my
ambience / system. Please see the result that I obtained.

=======================================================

TEST                           RESULT         TIME (ms)
-------------------------------------------------------

backup.backup_functions 'innodb' [ pass ]           1336
backup.backup_procedures 'innodb' [ pass ]            420
backup.backup_triggers 'innodb' [ pass ]           1831
backup.backup_functions 'falcon' [ pass ]           1767
backup.backup_procedures 'falcon' [ pass ]            459
backup.backup_triggers 'falcon' [ pass ]           2277
backup.backup_functions 'myisam' [ pass ]           1042
backup.backup_procedures 'myisam' [ pass ]            422
backup.backup_triggers 'myisam' [ pass ]           1462
backup.backup_functions 'memory' [ pass ]           1008
backup.backup_procedures 'memory' [ pass ]            394
backup.backup_triggers 'memory' [ skipped ]   Temporarily Disabled Bug#35249
---------------------------------------


> Chuck
> 
> 
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
> 
> 

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