MySQL Proxy

Code review comment for lp:~jan-kneschke/mysql-proxy/test-suite-refactoring

test-suite-refactoring
Merge into trunk

Revision history for this message

Jan Kneschke (jan-kneschke) wrote on 2010-09-07:

Am 07.09.2010 um 11:41 schrieb Leith:

> Review: Needs Fixing
> Looks good to me. A few very minor comments (with the first being the only one needing to be resolved):
>
> - No copyright/licenses headers have had the date updated.. Please do that

added the copyright headers to all the testenv/ files.

> - Can we keep this comment on one line:
> + /* only try to delete the PID if we created it
> + */

done.

> - Why hard code the following to MySQLProxy:get_args()?:
> + opts["plugins"] = "proxy"
> + opts["daemon"] = true

because we only want to run the 'proxy' plugin and want to run it all as daemon all the time.

We don't want the admin plugin to be loaded here as it would only make it more complicated to setup the ports for it. As we don't use it, we don't load it for these tests.

> - This looks incomplete, what is the plan here?:
> + if extra_params then
> + -- FIXME: implement me
> + end

I removed it.

> - Bug#38415 has been fixed now (https://code.launchpad.net/~schuster/mysql-proxy/remove_unix_socket/+merge/23652), we can just remove the following now:
>
> + -- if we are supposed to listen on a UNIX socket, remove it first, because we don't clean it up on exit!
> + -- TODO: fix the code, instead of hacking around here! Bug#38415
> + if proxy_options['proxy-address'] == '/tmp/mysql-proxy-test.sock' then
> + os.remove(proxy_options['proxy-address'])
> + end

Will post-pone remove this until #38415 is marked as "closed".

> - I see changes to Makefile.am, but none to any CMakeLists.txt files, I understand that the test suite does not run on Windows yet, however I'd also like us to move towards cmake as the one true source as well. Please consider making it so that cmake can still build all of this on *nix, and opening a new issue to get the test suite working on Windows as well.

Let's move this into an extra task. The current suite has only files for the automake case.

> --
> https://code.launchpad.net/~jan-kneschke/mysql-proxy/test-suite-refactoring/+merge/34720
> You are the owner of lp:~jan-kneschke/mysql-proxy/test-suite-refactoring.

Jan

Am 07.09.2010 um 11:41 schrieb Leith:

> Review: Needs Fixing
> Looks good to me. A few very minor comments (with the first being the only one needing to be resolved):
> 
> - No copyright/licenses headers have had the date updated.. Please do that

added the copyright headers to all the testenv/ files.

> - Can we keep this comment on one line:
> 	+		/* only try to delete the PID if we created it
> 	+		 */

done.

> - Why hard code the following to MySQLProxy:get_args()?:
> 	+	opts["plugins"]      = "proxy"
> 	+	opts["daemon"]       = true

because we only want to run the 'proxy' plugin and want to run it all as daemon all the time.

We don't want the admin plugin to be loaded here as it would only make it more complicated to setup the ports for it. As we don't use it, we don't load it for these tests.

> - This looks incomplete, what is the plan here?:
> 	+	if extra_params then
> 	+		-- FIXME: implement me
> 	+	end

I removed it.

> - Bug#38415 has been fixed now (https://code.launchpad.net/~schuster/mysql-proxy/remove_unix_socket/+merge/23652), we can just remove the following now:
> 
> 	+	-- if we are supposed to listen on a UNIX socket, remove it first, because we don't clean it up on exit!
> 	+	-- TODO: fix the code, instead of hacking around here! Bug#38415
> 	+	if proxy_options['proxy-address'] == '/tmp/mysql-proxy-test.sock' then
> 	+		os.remove(proxy_options['proxy-address'])
> 	+	end

Will post-pone remove this until #38415 is marked as "closed".

Let's move this into an extra task. The current suite has only files for the automake case.

> -- 
> https://code.launchpad.net/~jan-kneschke/mysql-proxy/test-suite-refactoring/+merge/34720
> You are the owner of lp:~jan-kneschke/mysql-proxy/test-suite-refactoring.

Jan

« Back to merge proposal