Code review comment for lp:~capttofu/drizzle/memcached-udf-current

Revision history for this message
Padraig O'Sullivan (posulliv) wrote :

So for the test suite, I checked it out and the issue is that the result file seems to not have been updated after some UDF's return types were changed? When I run the test suite, I see there are a few diff's (this is just a sampling, there are more diff's):

 memc_delete('mysql:doc1')
-0
+1
 select memc_get('mysql:doc1');
 memc_get('mysql:doc1')
 NULL
 select memc_set('spot:test', ' Spot ');
 memc_set('spot:test', ' Spot ')
-0
+1
 select memc_get('spot:test');
 memc_get('spot:test')
-NULL
+ Spot
 select memc_prepend('spot:test', 'See');
 memc_prepend('spot:test', 'See')
-0
+1
 select memc_get('spot:test');
 memc_get('spot:test')
-NULL
+See Spot

It looks like the result file was recorded on a test run when all these UDF's failed! To record the result file, do the following:

$ ./dtr --record --suite=memcached_udf memc

Since I wrote the test suite, I should comment on some issues I see with the test suite:

 * it requires memcached to be in your path before running
 * it always starts memcached on the same port so if memcached is already running on this port, bad things will happen
 * the test cases are hard-coded to use a memcached instance on this port

I guess the major question I have here is how to have a test case with the ability to take an input parameter that can affect the result output (in this case, the port number to start memcached on)? I'm assuming its not possible right now unless we pass the parameter as an environment variable but I don't know how to access an environment variable from a test case?

Also, Monty's comment on not using %llu also needs to be implemented. Not sure how this branch compiled cleanly with that in there? I had to modify that to compile the branch and run the test suite.

-Padraig

review: Needs Fixing

« Back to merge proposal