Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77cd8ef
Print all failed test in a test suite which is an arr.ai tuple.
lordofpm Sep 5, 2020
05628d5
Merge branch 'master' into test_enhancement
ericzhang6222 Sep 6, 2020
bbdd36f
Added new func evalTests for Tests:
lordofpm Sep 6, 2020
cea3f75
Added new func evalTests for Tests:
lordofpm Sep 6, 2020
fd90cab
Added new func evalTests for Tests:
lordofpm Sep 6, 2020
fe55ff7
Merge branch 'master' into test_enhancement
ericzhang6222 Sep 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion internal/test/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import (

// Test runs all tests in the subtree of path and returns the results.
func Test(ctx context.Context, w io.Writer, path string) (Results, error) {
// Identifies it is running cmd test
ctx = context.WithValue(ctx, rel.CmdIdentifier{Name: "test"}, "test")

results := Results{}
var files []string
err := filepath.Walk(path, func(path string, info os.FileInfo, err error) error {
Expand All @@ -40,7 +43,7 @@ func Test(ctx context.Context, w io.Writer, path string) (Results, error) {
}
result, err := syntax.EvaluateExpr(ctx, file, string(bytes))
if err != nil {
fmt.Fprintf(w, "\nfailed test: %s\n", err)
fmt.Fprintf(w, "\nfailed test:\n%s\n", err)
results.Add(Result{file: file, pass: false})
} else {
results.Add(Result{file: file, pass: isRecursivelyTrue(result)})
Expand Down
27 changes: 27 additions & 0 deletions rel/expr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package rel
import (
"context"
"fmt"
"strings"

"github.com/arr-ai/wbnf/parser"
)
Expand Down Expand Up @@ -86,3 +87,29 @@ func EvalExpr(ctx context.Context, expr Expr, local Scope) (_ Value, err error)
//TODO: this is only the initial scope, how to get the last scope?
return expr.Eval(ctx, local)
}

// 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
}
Comment on lines +90 to +115
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

23 changes: 23 additions & 0 deletions rel/expr_tuple.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ func (e *TupleExpr) String() string { //nolint:dupl

// Eval returns the subject
func (e *TupleExpr) Eval(ctx context.Context, local Scope) (Value, error) {
if cmd, isStr := ctx.Value(CmdIdentifier{Name: "test"}).(string); isStr && cmd == "test" {
return e.evalTests(ctx, local)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

tuple := EmptyTuple
var err error
for _, attr := range e.attrs {
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my thinking is:

  1. The _test.arrai file should at least compile. If it doesn't you shouldn't expect good error reporting.
  2. 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).
  3. 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?

tuple := EmptyTuple
var err error
var errs Errors
for _, attr := range e.attrs {
tuple, err = attr.Apply(ctx, local, tuple)
if err != nil {
errs.errors = append(errs.errors, WrapContextErr(err, e, local))
tuple = EmptyTuple
}
}
if len(errs.errors) > 0 {
return nil, errs
}

// TODO: Construct new tuple directly
return tuple.(*GenericTuple).Canonical(), nil
}