GD-605: Fixes the CMD tool to collect multipe command arguments (#606)

# Why
The CMD tool had an issue to collect all given arguments. e.g. using
--add <value> or --i <value> multiple times it only applyes the first
argument example:
`./addons/gdUnit4/runtest.sh --add res://addons/gdUnit4/test/core/parse/
--add res://addons/gdUnit4/test/asserts/GdUnitAssertImplTest.gd -i
GdDefaultValueDecoderTest -i GdScriptParserTest`

# What
- fixes the root cause on `CmdCommandHandler#execute` to call the
correct callable to handle collected arguments
  - add register callback validations to the handler
- code formattings
- remove obsolete code `_enhanced_fr_test `

addons/gdUnit4/bin/GdUnitCmdTool.gd

@@ -312,12 +312,12 @@ class CLIRunner:
 		commands.append_array(result.value() as Array)
 		result = (
 			CmdCommandHandler.new(_cmd_options)
-				.register_cb("-help", Callable(self, "show_help"))
-				.register_cb("--help-advanced", Callable(self, "show_advanced_help"))
-				.register_cb("-a", Callable(_runner_config, "add_test_suite"))
-				.register_cbv("-a", Callable(_runner_config, "add_test_suites"))
-				.register_cb("-i", Callable(_runner_config, "skip_test_suite"))
-				.register_cbv("-i", Callable(_runner_config, "skip_test_suites"))
+				.register_cb("-help", show_help)
+				.register_cb("--help-advanced", show_advanced_help)
+				.register_cb("-a", _runner_config.add_test_suite)
+				.register_cbv("-a", _runner_config.add_test_suites)
+				.register_cb("-i", _runner_config.skip_test_suite)
+				.register_cbv("-i", _runner_config.skip_test_suites)
 				.register_cb("-rd", set_report_dir)
 				.register_cb("-rc", set_report_count)
 				.register_cb("--selftest", run_self_test)

addons/gdUnit4/src/cmd/CmdCommandHandler.gd

@@ -10,26 +10,28 @@ var _cmd_options :CmdOptions
 # Dictionary[String, Array[Callback]
 var _command_cbs :Dictionary
 
-# we only able to check cb function name since Godot 3.3.x
-var _enhanced_fr_test := false
 
 
 func _init(cmd_options: CmdOptions) -> void:
 	_cmd_options = cmd_options
-	var major: int = Engine.get_version_info()["major"]
-	var minor: int = Engine.get_version_info()["minor"]
-	if major == 3 and minor == 3:
-		_enhanced_fr_test = true
 
 
 # register a callback function for given command
 # cmd_name short name of the command
 # fr_arg a funcref to a function with a single argument
-func register_cb(cmd_name: String, cb: Callable = NO_CB) -> CmdCommandHandler:
+func register_cb(cmd_name: String, cb: Callable) -> CmdCommandHandler:
 	var registered_cb: Array = _command_cbs.get(cmd_name, [NO_CB, NO_CB])
 	if registered_cb[CB_SINGLE_ARG]:
 		push_error("A function for command '%s' is already registered!" % cmd_name)
 		return self
+
+	if not _validate_cb_signature(cb, TYPE_STRING):
+		push_error(
+			("The callback '%s:%s' for command '%s' has invalid function signature. "
+			+"The callback signature must be 'func name(value: PackedStringArray)'")
+			% [cb.get_object().get_class(), cb.get_method(), cmd_name])
+		return null
+
 	registered_cb[CB_SINGLE_ARG] = cb
 	_command_cbs[cmd_name] = registered_cb
 	return self
@@ -42,15 +44,23 @@ func register_cbv(cmd_name: String, cb: Callable) -> CmdCommandHandler:
 	if registered_cb[CB_MULTI_ARGS]:
 		push_error("A function for command '%s' is already registered!" % cmd_name)
 		return self
+
+	if not _validate_cb_signature(cb, TYPE_PACKED_STRING_ARRAY):
+		push_error(
+			("The callback '%s:%s' for command '%s' has invalid function signature. "
+			+"The callback signature must be 'func name(value: PackedStringArray)'")
+			% [cb.get_object().get_class(), cb.get_method(), cmd_name])
+		return null
+
 	registered_cb[CB_MULTI_ARGS] = cb
 	_command_cbs[cmd_name] = registered_cb
 	return self
 
 
 func _validate() -> GdUnitResult:
-	var errors: = PackedStringArray()
+	var errors := PackedStringArray()
 	# Dictionary[StringName, String]
-	var registered_cbs: = Dictionary()
+	var registered_cbs := Dictionary()
 
 	for cmd_name in _command_cbs.keys() as Array[String]:
 		var cb: Callable = (_command_cbs[cmd_name][CB_SINGLE_ARG]
@@ -63,8 +73,8 @@ func _validate() -> GdUnitResult:
 			@warning_ignore("return_value_discarded")
 			errors.append("The command '%s' is unknown, verify your CmdOptions!" % cmd_name)
 		# verify for multiple registered command callbacks
-		if _enhanced_fr_test and cb != NO_CB:
-			var cb_method: = cb.get_method()
+		if cb != NO_CB:
+			var cb_method := cb.get_method()
 			if registered_cbs.has(cb_method):
 				var already_registered_cmd :String = registered_cbs[cb_method]
 				@warning_ignore("return_value_discarded")
@@ -76,33 +86,51 @@ func _validate() -> GdUnitResult:
 	return GdUnitResult.error("\n".join(errors))
 
 
-func execute(commands :Array[CmdCommand]) -> GdUnitResult:
+func execute(commands: Array[CmdCommand]) -> GdUnitResult:
 	var result := _validate()
 	if result.is_error():
 		return result
 	for cmd in commands:
 		var cmd_name := cmd.name()
 		if _command_cbs.has(cmd_name):
-			var cb_s :Callable = _command_cbs.get(cmd_name)[CB_SINGLE_ARG]
+			var cb_s: Callable = _command_cbs.get(cmd_name)[CB_SINGLE_ARG]
 			var arguments := cmd.arguments()
 			var cmd_option := _cmd_options.get_option(cmd_name)
-			if cb_s and arguments.size() == 0:
+
+			if arguments.is_empty():
 				cb_s.call()
-			elif cb_s:
+			elif arguments.size() > 1:
+				var cb_m: Callable = _command_cbs.get(cmd_name)[CB_MULTI_ARGS]
+				cb_m.call(arguments)
+			else:
 				if cmd_option.type() == TYPE_BOOL:
 					cb_s.call(true if arguments[0] == "true" else false)
 				else:
 					cb_s.call(arguments[0])
-			else:
-				var cb_m :Callable = _command_cbs.get(cmd_name)[CB_MULTI_ARGS]
-				# we need to find the method and determin the arguments to call the right function
-				for m in cb_m.get_object().get_method_list():
-					if m["name"] == cb_m.get_method():
-						@warning_ignore("unsafe_cast")
-						if (m["args"] as Array).size() > 1:
-							cb_m.callv(arguments)
-							break
-						else:
-							cb_m.call(arguments)
-							break
+
 	return GdUnitResult.success(true)
+
+
+func _validate_cb_signature(cb: Callable, arg_type: int) -> bool:
+	for m in cb.get_object().get_method_list():
+		if m["name"] == cb.get_method():
+			@warning_ignore("unsafe_cast")
+			return _validate_func_arguments(m["args"] as Array, arg_type)
+	return true
+
+
+func _validate_func_arguments(arguments: Array, arg_type: int) -> bool:
+	# validate we have a single argument
+	if arguments.size() > 1:
+		return false
+	# a cb with no arguments is also valid
+	if arguments.size() == 0:
+		return true
+	# validate argument type
+	var arg: Dictionary = arguments[0]
+	@warning_ignore("unsafe_cast")
+	if arg["usage"] as int == PROPERTY_USAGE_NIL_IS_VARIANT:
+		return true
+	if arg["type"] != arg_type:
+		return false
+	return true

addons/gdUnit4/src/core/GdUnitRunnerConfig.gd

@@ -8,7 +8,7 @@ const VERSION = "version"
 const INCLUDED = "included"
 const SKIPPED = "skipped"
 const SERVER_PORT = "server_port"
-const EXIT_FAIL_FAST ="exit_on_first_fail"
+const EXIT_FAIL_FAST = "exit_on_first_fail"
 
 const CONFIG_FILE = "res://addons/gdUnit4/GdUnitRunner.cfg"
 
@@ -29,7 +29,7 @@ func clear() -> GdUnitRunnerConfig:
 	return self
 
 
-func set_server_port(port :int) -> GdUnitRunnerConfig:
+func set_server_port(port: int) -> GdUnitRunnerConfig:
 	_config[SERVER_PORT] = port
 	return self
 
@@ -45,22 +45,22 @@ func self_test() -> GdUnitRunnerConfig:
 	return self
 
 
-func add_test_suite(p_resource_path :String) -> GdUnitRunnerConfig:
+func add_test_suite(p_resource_path: String) -> GdUnitRunnerConfig:
 	var to_execute_ := to_execute()
 	to_execute_[p_resource_path] = to_execute_.get(p_resource_path, PackedStringArray())
 	return self
 
 
-func add_test_suites(resource_paths :PackedStringArray) -> GdUnitRunnerConfig:
+func add_test_suites(resource_paths: PackedStringArray) -> GdUnitRunnerConfig:
 	for resource_path_ in resource_paths:
 		@warning_ignore("return_value_discarded")
 		add_test_suite(resource_path_)
 	return self
 
 
-func add_test_case(p_resource_path :String, test_name :StringName, test_param_index :int = -1) -> GdUnitRunnerConfig:
+func add_test_case(p_resource_path: String, test_name: String, test_param_index: int = -1) -> GdUnitRunnerConfig:
 	var to_execute_ := to_execute()
-	var test_cases :PackedStringArray = to_execute_.get(p_resource_path, PackedStringArray())
+	var test_cases: PackedStringArray = to_execute_.get(p_resource_path, PackedStringArray())
 	if test_param_index != -1:
 		@warning_ignore("return_value_discarded")
 		test_cases.append("%s:%d" % [test_name, test_param_index])
@@ -75,7 +75,7 @@ func add_test_case(p_resource_path :String, test_name :StringName, test_param_in
 # <test_suite_name|path>[:<test_case_name>]
 # '/path/path', res://path/path', 'res://path/path/testsuite.gd' or 'testsuite'
 # 'res://path/path/testsuite.gd:test_case' or 'testsuite:test_case'
-func skip_test_suite(value :StringName) -> GdUnitRunnerConfig:
+func skip_test_suite(value: String) -> GdUnitRunnerConfig:
 	var parts: PackedStringArray = GdUnitFileAccess.make_qualified_path(value).rsplit(":")
 	if parts[0] == "res":
 		parts.remove_at(0)
@@ -85,20 +85,20 @@ func skip_test_suite(value :StringName) -> GdUnitRunnerConfig:
 			skipped()[parts[0]] = PackedStringArray()
 		2:
 			@warning_ignore("return_value_discarded")
-			skip_test_case(parts[0], parts[1])
+			_skip_test_case(parts[0], parts[1])
 	return self
 
 
-func skip_test_suites(resource_paths :PackedStringArray) -> GdUnitRunnerConfig:
+func skip_test_suites(resource_paths: PackedStringArray) -> GdUnitRunnerConfig:
 	for resource_path_ in resource_paths:
 		@warning_ignore("return_value_discarded")
 		skip_test_suite(resource_path_)
 	return self
 
 
-func skip_test_case(p_resource_path :String, test_name :StringName) -> GdUnitRunnerConfig:
+func _skip_test_case(p_resource_path: String, test_name: String) -> GdUnitRunnerConfig:
 	var to_ignore := skipped()
-	var test_cases :PackedStringArray = to_ignore.get(p_resource_path, PackedStringArray())
+	var test_cases: PackedStringArray = to_ignore.get(p_resource_path, PackedStringArray())
 	@warning_ignore("return_value_discarded")
 	test_cases.append(test_name)
 	to_ignore[p_resource_path] = test_cases
@@ -114,7 +114,7 @@ func skipped() -> Dictionary:
 	return _config.get(SKIPPED, {})
 
 
-func save_config(path :String = CONFIG_FILE) -> GdUnitResult:
+func save_config(path: String = CONFIG_FILE) -> GdUnitResult:
 	var file := FileAccess.open(path, FileAccess.WRITE)
 	if file == null:
 		var error := FileAccess.get_open_error()
@@ -124,7 +124,7 @@ func save_config(path :String = CONFIG_FILE) -> GdUnitResult:
 	return GdUnitResult.success(path)
 
 
-func load_config(path :String = CONFIG_FILE) -> GdUnitResult:
+func load_config(path: String = CONFIG_FILE) -> GdUnitResult:
 	if not FileAccess.file_exists(path):
 		return GdUnitResult.error("Can't find test runner configuration '%s'! Please select a test to run." % path)
 	var file := FileAccess.open(path, FileAccess.READ)
@@ -148,13 +148,13 @@ func load_config(path :String = CONFIG_FILE) -> GdUnitResult:
 @warning_ignore("unsafe_cast")
 func fix_value_types() -> void:
 	# fix float value to int json stores all numbers as float
-	var server_port_ :int = _config.get(SERVER_PORT, -1)
+	var server_port_: int = _config.get(SERVER_PORT, -1)
 	_config[SERVER_PORT] = server_port_
 	convert_Array_to_PackedStringArray(_config[INCLUDED] as Dictionary)
 	convert_Array_to_PackedStringArray(_config[SKIPPED] as Dictionary)
 
 
-func convert_Array_to_PackedStringArray(data :Dictionary) -> void:
+func convert_Array_to_PackedStringArray(data: Dictionary) -> void:
 	for key in data.keys() as Array[String]:
 		var values :Array = data[key]
 		data[key] = PackedStringArray(values)