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

Revision history for this message
Leith (mleith) wrote :

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

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

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

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

 - 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

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

review: Needs Fixing

« Back to merge proposal