List:Commits« Previous MessageNext Message »
From:Hema Sridharan Date:July 2 2008 5:49am
Subject:RE: WL#4222 Test patch review
View as plain text  
Hi Chuck,

After our discussion this morning in IRC, I revisited the test case and I
want to share my thoughts with you. Replying you inline.

> Subject: RE: WL#4222 Test patch review
> 
> Hema,
> 
> My concern is the test will not run at all if you don't have 
> innodb or falcon. Basically, the test will only run in an 
> environment built as 'max'


Hema: This isn't true. If engines aren't present then we have to remove
those engines from the combinations file and test will run for the other
engines mentioned in the combinations file. By doing this, we don't have to
alter the test case based on build and keep the changes limited to the
combinations file only.


> -- something that is not likely to happen (IMHO for 
> developers). As a developer, that means I have to build my 
> system differently and, in the case of falcon, adds a 
> considerable amount to the build time. 
> 
> I think the tests should be able to be run on systems where 
> Falcon or InnoDB are not included. Please change the tests to 
> allow the tests to run if any of the engines are available.
> 

Hema: Your intention is to make the test independent of available engines
and we could accomplish that by modifying the combination file. Also the
purpose of keeping all these in the suite is to run them with all available
engines (if present in a particular build) and Lars also suggested the same
idea before.


> Also, please change the message printed by the test. It is 
> not intuitive.

Hema: Ok, agreed. I will do the necessary changes.

Overall both of us have the same target in mind and I would prefer to
accomplish that by modifying the combinations flle. Please let me know, if
you agree the above data points. 

Thanks & Regards,
Hema. S


> 
> Chuck
> 
> > > One more thing... I see that it is now impossible to run
> > these tests
> > > on a system that has only MyISAM built. The tests are
> > designed so that
> > > one has to have innodb, falcon, myisam, and memory 
> storage engines 
> > > built/installed in order to run the tests. I wonder if that
> > is such a
> > > good idea?
> > > 
> > 
> > Hema: It is always possible to remove the engines from the 
> > combinations file and run the tests with the specific 
> engine types.I 
> > don't see the necessity of removing have_falcon and have_innodb. 
> > Please advice and I push the patch once I get your approval.
> > 
> > Thanks,
> > Hema.S
> > 
> > > I think maybe you need to remove the have_falcon, 
> have_innodb, etc. 
> > > and handle it like you do above with the engines thing. 
> Only please 
> > > make a more informative message.
> > > 'Temporarily disabled' sounds like it is disabled as in
> > disabled.def
> > > file but it isn't. In this case, you should print a message
> > like 'Test
> > > skipped. No InnoDB support.'
> 
> 
> -- 
> 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