From 0f042124cb32deabb99f5512941ecb2a2bbf74f6 Mon Sep 17 00:00:00 2001
From: Delyan Angelov <delian66@gmail.com>
Date: Fri, 5 Mar 2021 13:19:39 +0200
Subject: [PATCH] tests: support `fn test_fn() ? { opt()? }`

---
 vlib/v/ast/ast.v                              |  2 ++
 vlib/v/checker/checker.v                      | 20 ++++++++++---------
 .../test_functions_should_not_return_test.out |  8 +-------
 .../test_functions_should_not_return_test.vv  |  4 +++-
 vlib/v/gen/c/cgen.v                           | 10 ++++------
 vlib/v/gen/c/cmain.v                          | 18 ++++++++++++++---
 vlib/v/parser/fn.v                            |  9 +++++++++
 vlib/v/parser/parser.v                        |  1 +
 vlib/v/pref/pref.v                            |  2 +-
 vlib/v/preludes/tests_assertions.v            | 18 +++++++++++++++++
 vlib/v/table/table.v                          |  2 ++
 11 files changed, 67 insertions(+), 27 deletions(-)

diff --git a/vlib/v/ast/ast.v b/vlib/v/ast/ast.v
index 930998823c..d3f5031ff7 100644
--- a/vlib/v/ast/ast.v
+++ b/vlib/v/ast/ast.v
@@ -317,6 +317,8 @@ pub:
 	is_variadic     bool
 	is_anon         bool
 	is_manualfree   bool // true, when [manualfree] is used on a fn
+	is_main         bool // true for `fn main()`
+	is_test         bool // true for `fn test_abcde`
 	receiver        Field
 	receiver_pos    token.Position // `(u User)` in `fn (u User) name()` position
 	is_method       bool
diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v
index 6e88f70b66..5656c94e1d 100644
--- a/vlib/v/checker/checker.v
+++ b/vlib/v/checker/checker.v
@@ -170,6 +170,7 @@ pub fn (mut c Checker) check_files(ast_files []ast.File) {
 			the_main_file.stmts << ast.FnDecl{
 				name: 'main.main'
 				mod: 'main'
+				is_main: true
 				file: the_main_file.path
 				return_type: table.void_type
 				scope: &ast.Scope{
@@ -205,7 +206,7 @@ pub fn (mut c Checker) check_files(ast_files []ast.File) {
 	if c.pref.is_test {
 		mut n_test_fns := 0
 		for _, f in c.table.fns {
-			if f.name.contains('.test_') {
+			if f.is_test {
 				n_test_fns++
 			}
 		}
@@ -1213,8 +1214,7 @@ pub fn (mut c Checker) call_expr(mut call_expr ast.CallExpr) table.Type {
 	c.expected_or_type = table.void_type
 	if call_expr.or_block.kind == .propagate && !c.cur_fn.return_type.has_flag(.optional)
 		&& !c.inside_const {
-		cur_names := c.cur_fn.name.split('.')
-		if cur_names[cur_names.len - 1] != 'main' {
+		if !c.cur_fn.is_main {
 			c.error('to propagate the optional call, `$c.cur_fn.name` must return an optional',
 				call_expr.or_block.pos)
 		}
@@ -5440,7 +5440,9 @@ pub fn (mut c Checker) enum_val(mut node ast.EnumVal) table.Type {
 	// rintln('checker: x = $info.x enum val $c.expected_type $typ_sym.name')
 	// println(info.vals)
 	if node.val !in info.vals {
-		c.error('enum `$typ_sym.name` does not have a value `$node.val`', node.pos)
+		suggestion := util.new_suggestion(node.val, info.vals)
+		c.error(suggestion.say('enum `$typ_sym.name` does not have a value `$node.val`'),
+			node.pos)
 	}
 	node.typ = typ
 	return typ
@@ -5916,8 +5918,7 @@ fn (mut c Checker) fn_decl(mut node ast.FnDecl) {
 		}
 	}
 	// TODO c.pref.is_vet
-	if node.language == .v && !node.is_method && node.params.len == 0
-		&& node.name.after('.').starts_with('test_') {
+	if node.language == .v && !node.is_method && node.params.len == 0 && node.is_test {
 		if !c.pref.is_test {
 			// simple heuristic
 			for st in node.stmts {
@@ -5928,9 +5929,10 @@ fn (mut c Checker) fn_decl(mut node ast.FnDecl) {
 				}
 			}
 		}
-		// eprintln('> node.name: $node.name | node.return_type: $node.return_type')
-		if node.return_type != table.void_type_idx {
-			c.error('test functions should not return anything', node.pos)
+		if node.return_type != table.void_type_idx
+			&& node.return_type.clear_flag(.optional) != table.void_type_idx {
+			c.error('test functions should either return nothing at all, or be marked to return `?`',
+				node.pos)
 		}
 	}
 	c.expected_type = table.void_type
diff --git a/vlib/v/checker/tests/test_functions_should_not_return_test.out b/vlib/v/checker/tests/test_functions_should_not_return_test.out
index 4442e75647..0972921368 100644
--- a/vlib/v/checker/tests/test_functions_should_not_return_test.out
+++ b/vlib/v/checker/tests/test_functions_should_not_return_test.out
@@ -1,13 +1,7 @@
-vlib/v/checker/tests/test_functions_should_not_return_test.vv:9:1: error: test functions should not return anything
+vlib/v/checker/tests/test_functions_should_not_return_test.vv:9:1: error: test functions should either return nothing at all, or be marked to return `?`
     7 | 
     8 | // should be disallowed:
     9 | fn test_returning_int() int {
       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~
    10 | 
    11 | }
-vlib/v/checker/tests/test_functions_should_not_return_test.vv:14:1: error: test functions should not return anything
-   12 | 
-   13 | // should be disallowed:
-   14 | fn test_returning_opt() ? {
-      | ~~~~~~~~~~~~~~~~~~~~~~~~~
-   15 | }
diff --git a/vlib/v/checker/tests/test_functions_should_not_return_test.vv b/vlib/v/checker/tests/test_functions_should_not_return_test.vv
index 3255a262f2..3bebcea061 100644
--- a/vlib/v/checker/tests/test_functions_should_not_return_test.vv
+++ b/vlib/v/checker/tests/test_functions_should_not_return_test.vv
@@ -10,6 +10,8 @@ fn test_returning_int() int {
 
 }
 
-// should be disallowed:
+// NB: this is allowed explicitly now, to allow for shorter tests
+// of functions returning optionals.
 fn test_returning_opt() ? {
+	assert true
 }
diff --git a/vlib/v/gen/c/cgen.v b/vlib/v/gen/c/cgen.v
index cd48a56493..5d62d7da01 100644
--- a/vlib/v/gen/c/cgen.v
+++ b/vlib/v/gen/c/cgen.v
@@ -5252,7 +5252,7 @@ fn (mut g Gen) or_block(var_name string, or_block ast.OrExpr, return_type table.
 			g.stmts(stmts)
 		}
 	} else if or_block.kind == .propagate {
-		if g.file.mod.name == 'main' && (isnil(g.fn_decl) || g.fn_decl.name == 'main.main') {
+		if g.file.mod.name == 'main' && (isnil(g.fn_decl) || g.fn_decl.is_main) {
 			// In main(), an `opt()?` call is sugar for `opt() or { panic(err) }`
 			if g.pref.is_debug {
 				paline, pafile, pamod, pafn := g.panic_debug_info(or_block.pos)
@@ -5260,6 +5260,8 @@ fn (mut g Gen) or_block(var_name string, or_block ast.OrExpr, return_type table.
 			} else {
 				g.writeln('\tv_panic(_STR("optional not set (%.*s\\000)", 2, ${cvar_name}.err.msg));')
 			}
+		} else if !isnil(g.fn_decl) && g.fn_decl.is_test {
+			g.gen_failing_error_propagation_for_test_fn(or_block, cvar_name)
 		} else {
 			// In ordinary functions, `opt()?` call is sugar for:
 			// `opt() or { return err }`
@@ -5455,11 +5457,7 @@ fn (g &Gen) get_all_test_function_names() []string {
 	if tsuite_end.len > 0 {
 		all_tfuncs << tsuite_end
 	}
-	mut all_tfuncs_c := []string{}
-	for f in all_tfuncs {
-		all_tfuncs_c << util.no_dots(f)
-	}
-	return all_tfuncs_c
+	return all_tfuncs
 }
 
 fn (g &Gen) is_importing_os() bool {
diff --git a/vlib/v/gen/c/cmain.v b/vlib/v/gen/c/cmain.v
index 2b20fb9c4f..b8aca0d0f8 100644
--- a/vlib/v/gen/c/cmain.v
+++ b/vlib/v/gen/c/cmain.v
@@ -1,6 +1,7 @@
 module c
 
 import v.util
+import v.ast
 
 pub fn (mut g Gen) gen_c_main() {
 	if !g.has_main {
@@ -135,6 +136,16 @@ pub fn (mut g Gen) write_tests_definitions() {
 	g.definitions.writeln('jmp_buf g_jump_buffer;')
 }
 
+pub fn (mut g Gen) gen_failing_error_propagation_for_test_fn(or_block ast.OrExpr, cvar_name string) {
+	// in test_() functions, an `opt()?` call is sugar for
+	// `or { cb_propagate_test_error(@LINE, @FILE, @MOD, @FN, err.msg) }`
+	// and the test is considered failed
+	paline, pafile, pamod, pafn := g.panic_debug_info(or_block.pos)
+	g.writeln('\tmain__cb_propagate_test_error($paline, tos3("$pafile"), tos3("$pamod"), tos3("$pafn"), ${cvar_name}.err.msg );')
+	g.writeln('\tg_test_fails++;')
+	g.writeln('\tlongjmp(g_jump_buffer, 1);')
+}
+
 pub fn (mut g Gen) gen_c_main_for_tests() {
 	main_fn_start_pos := g.out.len
 	g.writeln('')
@@ -145,11 +156,12 @@ pub fn (mut g Gen) gen_c_main_for_tests() {
 		g.writeln('\tmain__BenchedTests bt = main__start_testing($all_tfuncs.len, _SLIT("$g.pref.path"));')
 	}
 	g.writeln('')
-	for t in all_tfuncs {
+	for tname in all_tfuncs {
+		tcname := util.no_dots(tname)
 		if g.pref.is_stats {
-			g.writeln('\tmain__BenchedTests_testing_step_start(&bt, _SLIT("$t"));')
+			g.writeln('\tmain__BenchedTests_testing_step_start(&bt, _SLIT("$tcname"));')
 		}
-		g.writeln('\tif (!setjmp(g_jump_buffer)) ${t}();')
+		g.writeln('\tif (!setjmp(g_jump_buffer)) ${tcname}();')
 		if g.pref.is_stats {
 			g.writeln('\tmain__BenchedTests_testing_step_end(&bt);')
 		}
diff --git a/vlib/v/parser/fn.v b/vlib/v/parser/fn.v
index c40696c53b..ec6e3fc331 100644
--- a/vlib/v/parser/fn.v
+++ b/vlib/v/parser/fn.v
@@ -296,6 +296,9 @@ fn (mut p Parser) fn_decl() ast.FnDecl {
 	mut type_sym_method_idx := 0
 	no_body := p.tok.kind != .lcbr
 	end_pos := p.prev_tok.position()
+	short_fn_name := name
+	is_main := short_fn_name == 'main' && p.mod == 'main'
+	is_test := short_fn_name.starts_with('test_') || short_fn_name.starts_with('testsuite_')
 	// Register
 	if is_method {
 		mut type_sym := p.table.get_type_symbol(rec.typ)
@@ -326,6 +329,8 @@ fn (mut p Parser) fn_decl() ast.FnDecl {
 			is_pub: is_pub
 			is_deprecated: is_deprecated
 			is_unsafe: is_unsafe
+			is_main: is_main
+			is_test: is_test
 			no_body: no_body
 			mod: p.mod
 			attrs: p.attrs
@@ -351,6 +356,8 @@ fn (mut p Parser) fn_decl() ast.FnDecl {
 			is_pub: is_pub
 			is_deprecated: is_deprecated
 			is_unsafe: is_unsafe
+			is_main: is_main
+			is_test: is_test
 			no_body: no_body
 			mod: p.mod
 			attrs: p.attrs
@@ -388,6 +395,8 @@ fn (mut p Parser) fn_decl() ast.FnDecl {
 		is_direct_arr: is_direct_arr
 		is_pub: is_pub
 		is_variadic: is_variadic
+		is_main: is_main
+		is_test: is_test
 		receiver: ast.Field{
 			name: rec.name
 			typ: rec.typ
diff --git a/vlib/v/parser/parser.v b/vlib/v/parser/parser.v
index 131b8b980b..01d33d9ec3 100644
--- a/vlib/v/parser/parser.v
+++ b/vlib/v/parser/parser.v
@@ -556,6 +556,7 @@ pub fn (mut p Parser) top_stmt() ast.Stmt {
 					return ast.FnDecl{
 						name: 'main.main'
 						mod: 'main'
+						is_main: true
 						stmts: stmts
 						file: p.file_name
 						return_type: table.void_type
diff --git a/vlib/v/pref/pref.v b/vlib/v/pref/pref.v
index 774a40d485..a37898a4e1 100644
--- a/vlib/v/pref/pref.v
+++ b/vlib/v/pref/pref.v
@@ -350,7 +350,7 @@ pub fn parse_args(known_external_commands []string, args []string) (&Preferences
 				res.build_options << '$arg $target_os'
 			}
 			'-printfn' {
-				res.printfn_list << cmdline.option(current_args, '-printfn', '')
+				res.printfn_list << cmdline.option(current_args, '-printfn', '').split(',')
 				i++
 			}
 			'-cflags' {
diff --git a/vlib/v/preludes/tests_assertions.v b/vlib/v/preludes/tests_assertions.v
index b665121fdb..2675cf1fed 100644
--- a/vlib/v/preludes/tests_assertions.v
+++ b/vlib/v/preludes/tests_assertions.v
@@ -86,3 +86,21 @@ fn cb_assertion_ok(i &VAssertMetaInfo) {
 	println('$final_funcname ($final_filepath)')
 	*/
 }
+
+fn cb_propagate_test_error(line_nr int, file string, mod string, fn_name string, errmsg string) {
+	filepath := if use_relative_paths { file } else { os.real_path(file) }
+	mut final_filepath := filepath + ':$line_nr:'
+	if use_color {
+		final_filepath = term.gray(final_filepath)
+	}
+	mut final_funcname := 'fn ' + fn_name.replace('main.', '').replace('__', '.')
+	if use_color {
+		final_funcname = term.red('✗ ' + final_funcname)
+	}
+	final_msg := if use_color { term.dim(errmsg) } else { errmsg }
+	eprintln('$final_filepath $final_funcname failed propagation with error: $final_msg')
+	if os.is_file(file) {
+		source_lines := os.read_lines(file) or { []string{len: line_nr + 1} }
+		eprintln('${line_nr:5} | ${source_lines[line_nr - 1]}')
+	}
+}
diff --git a/vlib/v/table/table.v b/vlib/v/table/table.v
index cc794ed979..029a41b429 100644
--- a/vlib/v/table/table.v
+++ b/vlib/v/table/table.v
@@ -34,6 +34,8 @@ pub:
 	is_deprecated  bool
 	is_unsafe      bool
 	is_placeholder bool
+	is_main        bool
+	is_test        bool
 	no_body        bool
 	mod            string
 	ctdefine       string // compile time define. "myflag", when [if myflag] tag