Print all failed test in a test suite which is an arr.ai tuple. #631

ericzhang6222 · 2020-09-05T09:25:21Z

Fixes # .

Changes proposed in this pull request:

Print all failed test cases in an arr.ai test file

$ ./arrai test
Tests:
contrib/json_test.arrai
contrib/util_test.arrai
contrib/xml_test.arrai
examples/test/multiple_cases_test.arrai
examples/test/single_case_test.arrai

failed test:
not equal
expected: Hello, arrai!
actual:   Hello22, arrai!

examples/test/multiple_cases_test.arrai:4:18:
    # Added expression let to clarify the actual result can be a complex expression
    testCase1: //test.assert.equal("Hello, arrai!", let words = ["Hello22", "arrai!"];//seq.join(", ", words)),

unexpected size
expected: 2
actual:   {222, Hello, arrai!}

examples/test/multiple_cases_test.arrai:6:18:
    testCase2: //test.assert.unequal("Hello, arrai!", //seq.join(", ", ["Nihao", "arrai!"])),
    testCase3: //test.assert.size(2, {"Hello", "arrai!", "222"}),
examples/test/multiple_cases_test.arrai failed
4/5 tests passed

Checklist:

Added related tests
Made corresponding changes to the documentation

rel/expr.go

rel/expr_tuple.go

coveralls · 2020-09-06T03:05:34Z

Pull Request Test Coverage Report for Build 656b2ecd1faae2d5c274d338996f403805eb1a8d-PR-631

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 15 (0.0%) changed or added relevant lines in 2 files are covered.
6 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.04%) to 48.088%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rel/expr_tuple.go	0	5	0.0%
rel/expr.go	0	10	0.0%

Files with Coverage Reduction	New Missed Lines	%
rel/expr_tuple.go	2	0.0%
rel/value_set_str.go	4	57.87%

Totals
Change from base Build f579a6eeb1c4056709845436309157fc2799baa3:	-0.04%
Covered Lines:	5018
Relevant Lines:	10435

💛 - Coveralls

cmd/arrai/multiple_cases_test.arrai contrib/json_test.arrai contrib/util_test.arrai contrib/xml_test.arrai examples/test/multiple_cases_test.arrai examples/test/single_case_test.arrai failed test: not equal expected: Hello, arrai! actual: Hello22, arrai! �[1;37mcmd/arrai/multiple_cases_test.arrai:4:18:�[0m # Added expression let to clarify the actual result can be a complex expression testCase1: //�[1;31mtest.assert.equal("Hello, arrai!", let words = ["Hello22", "arrai!"];//seq.join(", ", words))�[0m, unexpected size expected: 1 actual: {Hello, arrai!} �[1;37mcmd/arrai/multiple_cases_test.arrai:6:18:�[0m testCase2: //test.assert.unequal("Hello, arrai!", //seq.join(", ", ["Nihao", "arrai!"])), testCase3: //�[1;31mtest.assert.size(1, {"Hello", "arrai!"})�[0m, cmd/arrai/multiple_cases_test.arrai failed 5/6 tests passed.

marcelocantos · 2020-09-06T22:35:01Z

rel/expr_tuple.go

+	if cmd, isStr := ctx.Value(CmdIdentifier{Name: "test"}).(string); isStr && cmd == "test" {
+		return e.evalTests(ctx, local)
+	}
+


This shouldn't be required. test.Test should explicitly unpack the TupleExpr and evaluate each component.

type TupleExpr struct { ExprScanner attrs []AttrExpr attrMap map[string]Expr }

This is current definition of TupleExpr, so it has to change attrs to Attrs to unpack in test.Test. Is it acceptable? Or it can add new functions in expr_tuple.go or package rel for test.

orlade-anz · 2020-09-10T04:23:55Z

rel/expr.go

+
+// Errors is sequence of error
+type Errors struct {
+	errors []error
+}
+
+// Error returns string represents Errors
+func (errs Errors) Error() string {
+	if len(errs.errors) == 0 {
+		return ""
+	}
+
+	var result strings.Builder
+	for i, err := range errs.errors {
+		if i > 0 {
+			result.WriteString("\n\n")
+		}
+		result.WriteString(err.Error())
+	}
+	return result.String()
+}
+
+// CmdIdentifier identifies an arrai command like test, shell and eval etc.
+type CmdIdentifier struct {
+	Name string
+}


This should live in internal/test/, since it should only be used in the case of testing.

In general, test-related code should not be mixed in with implementation code.

orlade-anz · 2020-09-10T04:24:57Z

rel/expr_tuple.go

@@ -151,3 +155,22 @@ func (e *TupleExpr) Eval(ctx context.Context, local Scope) (Value, error) {
 	// TODO: Construct new tuple directly
 	return tuple.(*GenericTuple).Canonical(), nil
 }
+
+func (e *TupleExpr) evalTests(ctx context.Context, local Scope) (Value, error) {


As above, this should be in internal/test. It could probably be an extension of the isRecursivelyTrue function, rather than a separate thing (i.e. in a test, we would want to report all of the errors at all levels of a data structure, not just for tuples and not just one level deep).

Current code can't support this approach. The code value, err := expr.Eval(ctx, scope) calculates the value, as its implementation when one error happens in evaluation, it will stop executing and return a nil as value. So it can't calculate the value with some code like isRecursivelyTrue.
And as current expr definition, it can't do the similar thing like isRecursivelyTrue too, refers #631 (comment).
I think it has to do a leverage, open access to attributes in expr or add some functions in package rel which can be used by test.

So my thinking is:

The _test.arrai file should at least compile. If it doesn't you shouldn't expect good error reporting.

If the test compiles, then its output will be a data structure: a set, array, dict or tuple (assume for now it's not just a single bool, though that should be allowed).

Consider the data structure as a tree: it has some branches (internal tree-shaped nodes) and some leaves (primitives, probably booleans). For each leaf that is not true, we want to report a failure and include the path through the branches to that leaf, the comparison that failed, and what the expected and actual values were.

To do this, we first evaluate the file and get the data structure and walk through all of the nodes. For each leaf node, add an Entry to a list with the path we took to get there. If the leaf is not true, then we do some more work to complete the report.

I'm going to make a key assumption here because it simplifies things a lot and will be largely true in practice. Assume that the data structure containing the test output is statically defined (i.e. a literal, not generated by some other code). Each branch is either a set, array, dict or tuple, and each leaf is some other kind of expression.

So to get the failed comparison, parse the test file and walk the AST in search of the lowest tuple or sequence attribute found in the failed path. If there are multiple (e.g. empty which could be used in different cases), work your way back up through the parent nodes to find the right one.

Once you have the leaf node (unevaluated at this point), search inside that for a comparison operator (something that returns true: =, >, <:, (<=), etc.). When you find one, replace it with some analogue to //test.assert* which, instead of returning true or false, evaluates to a tuple with the data you want to report (expected source and value, actual source and value).

Evaluate that modified AST and walk it, compiling the report from the leaf tuples. Fin.

I expect that I'm glossing over the complexity of "find the comparison", but it's conceptually possible.

You might argue "wow that's hard; this PR is an improvement, shouldn't we just merge it and do the more complete solution as a follow-up?" Maybe. I'm wary because a) it's mixing test code with implementation which is a code smell for both test and implementation code, and b) it's not in the same direction as the solution described above; we would ultimately plan to delete this test code, so why commit it in the first place and add an extra complexity and maintenance burden?

Print all failed test in a test suite which is an arr.ai tuple.

77cd8ef

ericzhang6222 requested a review from orlade-anz September 5, 2020 09:25

marcelocantos requested changes Sep 5, 2020

View reviewed changes

rel/expr.go Outdated Show resolved Hide resolved

rel/expr.go Outdated Show resolved Hide resolved

rel/expr_tuple.go Outdated Show resolved Hide resolved

rel/expr_tuple.go Outdated Show resolved Hide resolved

Merge branch 'master' into test_enhancement

05628d5

lordofpm and others added 4 commits September 6, 2020 23:46

Merge branch 'master' into test_enhancement

fe55ff7

ericzhang6222 requested a review from marcelocantos September 6, 2020 16:19

marcelocantos requested changes Sep 6, 2020

View reviewed changes

orlade-anz suggested changes Sep 10, 2020

View reviewed changes

ericzhang6222 marked this pull request as draft September 14, 2020 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print all failed test in a test suite which is an arr.ai tuple. #631

Print all failed test in a test suite which is an arr.ai tuple. #631

ericzhang6222 commented Sep 5, 2020

coveralls commented Sep 6, 2020 •

edited

Loading

marcelocantos Sep 6, 2020

ericzhang6222 Sep 13, 2020

orlade-anz Sep 10, 2020

orlade-anz Sep 10, 2020

ericzhang6222 Sep 13, 2020

orlade-anz Sep 14, 2020

Print all failed test in a test suite which is an arr.ai tuple. #631

Are you sure you want to change the base?

Print all failed test in a test suite which is an arr.ai tuple. #631

Conversation

ericzhang6222 commented Sep 5, 2020

coveralls commented Sep 6, 2020 • edited Loading

Pull Request Test Coverage Report for Build 656b2ecd1faae2d5c274d338996f403805eb1a8d-PR-631

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

marcelocantos Sep 6, 2020

Choose a reason for hiding this comment

ericzhang6222 Sep 13, 2020

Choose a reason for hiding this comment

orlade-anz Sep 10, 2020

Choose a reason for hiding this comment

orlade-anz Sep 10, 2020

Choose a reason for hiding this comment

ericzhang6222 Sep 13, 2020

Choose a reason for hiding this comment

orlade-anz Sep 14, 2020

Choose a reason for hiding this comment

coveralls commented Sep 6, 2020 •

edited

Loading