Allow lazily-evaluated code as argument to TEST

[d-torrance]

This is a quick followup to #3419, which changed TEST from a method to a keyword so that we could do a better job of keeping track of the location of tests.

In that PR, I gave it a very low binding strength (the same as elapsedTime, etc.) But this means that it behaves much differently than it did than it was a method, e.g.,:

i1 : TEST "assert(1 + 1 " | "== 2)"

i2 : toString tests(0, User)

o2 = assert(1 + 1 == 2)

In M2 1.24.05, when TEST was still a method, we got an error:

i1 : TEST "assert(1 + 1 " | "==2)"
stdio:1:22:(3): error: no method for binary operator | applied to objects:
--            null (of class Nothing)
--      |     "==2)" (of class String)

This is because SPACE has higher binding strength than |.

We increase the binding strength of TEST to just below SPACE so that it behaves essentially the same as it did when it was a method. We keep it a bit below to mimic the right associativity of SPACE, i.e., we want TEST f x to parse as TEST (f x), not (TEST f) x.

[mahrud]

I understand wanting it to behave the same as before, but is there a reason why the previous behavior was better?

[d-torrance]

This is maybe pretty artificial, but this is the current behavior, which seems wrong:

i1 : Nothing + Nothing := x -> "foo";

i2 : TEST "a" + TEST "b"
stdio:2:9:(3): error: no method for binary operator + applied to objects:
            "a" (of class String)
      +     null (of class Nothing)

It seems like we should get "foo" here.

[mahrud]

I can't imagine any use for the output of TEST.

I can, however, imagine setting up TEST to act as a delayed execution keyword, so we could do:

TEST 1 + 1 == 2

Without bothering with strings or parentheses.

[d-torrance]

You've convinced me! TEST ... should probably behave like () -> ....

I may try and implement writing tests w/o strings like this in this PR...

[mahrud]

TEST ... should probably behave like () -> ....

Yes, exactly, construct a TestInput object, store it in a list, and maybe also return it?

By the way, I'd like assert to behave similarly, and in particular I want to be able to sprinkle tons of assertions in my code and run them only depending on the value of debugLevel or assertLevel or something like that. It's possible that, with some work, we could get a interpreter-level assert keyword to also print extra debug information, in the spirit of how you use Equality expressions. I'll just turn this into a new issue ...

[d-torrance]

I have a very early draft where TEST ... behaves essentially like () -> ....

TestInput is now a subclass of FunctionClosure. We run it, and if it returns a string, then we treat it like a traditional test. Otherwise, we're done!

Tests defined in this way are very fast:

i1 : TEST "assert true"

i2 : TEST assert true

i3 : check User
 -- capturing check(0, "User")                   -- .0741398s elapsed
 -- calling   check(1, "User")                   -- .000079301s elapsed

[mahrud]

Tests defined in this way are very fast:

Interesting .. because we've front loaded the parsing. That might make benchmarking easier also (maybe benchmark should be a keyword as well?).

We should be careful that locality of symbols doesn't change in a bad way.

TestInput is now a subclass of FunctionClosure.

Then maybe rename it TestClosure?

[d-torrance]

We should be careful that locality of symbols doesn't change in a bad way.

It seems to work pretty well -- it's really no different than writing a function in a package.

I adapted the tests in TerraciniLoci as a proof of concept. The big differences were changing ='s to :='s to avoid "unexported mutable symbol" errors when loading the package, adding semicolons, and adding other packages used in the tests to PackageImports instead of calling needsPackage in the test itself since we need those symbols to be available when we first load the package.

The only breaking change to existing tests that I've encountered is the handful of tests that use TEST get(...). We won't call get until runtime, so using currentFileDirectory won't work. But changing that to PackageName#"auxiliary files" does the trick.

One thing I haven't quite figured out how to deal with is the desc field in the functionBody object. Currently, dummyDesc works since it specifies 0 arguments, which is what we want, but there's some frame stuff in there that's probably important.

[d-torrance]

One strange thing I haven't figured out how to fix yet:

i1 : TEST (f := x -> x^2)

i2 : TEST (f := x -> x^3)
stdio:2:6:(3): warning: local declaration of f shields variable with same name

[mahrud]

I think you need to define a local dictionary for each TestClosure.

[mahrud]

The only breaking change to existing tests that I've encountered is the handful of tests that use TEST get(...). We won't call get until runtime, so using currentFileDirectory won't work. But changing that to PackageName#"auxiliary files" does the trick.

I think this is not just testing that you can read the file, without loading it, so probably it should be changed to load.

Also, I think now looking at the code of the test just shows the load line rather than the actual contents. Not sure what's the correct solution here, maybe using something other than TEST to add test files?

[d-torrance]

I think this is not just testing that you can read the file, without loading it, so probably it should be changed to load.

I don't think we want to call load inside TEST. That would actually load the file, which maybe has undesired consequences. For example, suppose the file foo.m2 contains the single line x = 2. Then:

i1 : TEST load "foo.m2"

o1 = TestClosure[stdio:1:5-1:18]

o1 : TestClosure

i2 : check User
 -- calling   check(0, "User") -- .000138457s elapsed

i3 : x

o3 = 2

But with get, the test closure returns a string, which means we fall back on the traditional behavior:

i1 : TEST get "foo.m2"

o1 = TestClosure[stdio:1:5-1:17]

o1 : TestClosure

i2 : check User
 -- capturing check(0, "User")                   -- .0358661s elapsed

i3 : x

o3 = x

o3 : Symbol

Also, I think now looking at the code of the test just shows the load line rather than the actual contents. Not sure what's the correct solution here, maybe using something other than TEST to add test files?

That's exactly what addTest(String) does (currently unexported) for the Core tests. Theoretically, any package that has files in its currentLayout#"packagetests" directory will also load tests this way.

However, there's currently not a way to tell a package to put any files there, and the autotools/cmake builds have to manually do this for Core. I've toyed with the idea of implementing this -- maybe a TestFiles option for newPackage with a list/regex of test files to install there?

[d-torrance]

I've added a TestFiles option to newPackage so that files can be added as tests directly without using get or load, and locate points to the right place.

I also figured out how to give each TestClosure object its own local dictionary -- the TEST keyword now behaves much more like -> and is parsed as an Arrow parse tree object.

[mahrud]

This is complicating the testing system rather than simplifying it, and I would personally rather not add yet another option to newPackage (e.g. should we also add DocumentationFiles next?)

[d-torrance]

Fair enough -- I wasn't that excited about this idea either.

Another possibility would be to change currentLayout#"packagetests" to point to a subdirectory of the auxiliary files directory (say test or tests) instead of the current behavior, where it's a subdirectory of the package's documentation directory. Then any files in that directory would automatically get loaded as tests, as currently happens for Core.

[mahrud]

Polyhedra/tests might get in the way, but in principle that's fine with me.

[mahrud]

I don't understand why this is better. It's much more complicated than before.

[d-torrance]

The current behavior is to store a test's location in a hash table, which is easy to do at top level. But with this proposal, tests are becoming functions, and so unless we want all the tests that have been loaded from files to have testing.m2 as their location, we need to fake it somehow in the interpreter. This function was my attempt at doing that. I figured we should try to find out the ending position so that code will work.

[mahrud]

Treating TEST as -> is kludgy.

[d-torrance]

Yeah, it is a little strange. We need to call bind when things are still ParseTree objects in order to create a local dictionary, which is why I went this route. And Arrow is already set up for creating functions.

I also considered creating another member of ParseTree. Then we could avoid all the when lhs is dummy cases.

[mahrud]

I also figured out how to give each TestClosure object its own local dictionary -- the TEST keyword now behaves much more like -> and is parsed as an Arrow parse tree object.

I don't like this approach, and I don't think it needs to be this complicated.

I think we should keep TEST a unaryop and have it return an object of type:

export TestClosure := {+ frame:Frame, model:functionCode, hash:hash_t };

In particular, the model can be a normal functionCode, without having to change every step in the process.

[mahrud]

Could you make a branch on your fork with the last commit before you changed the behavior to act as an arrow? I see the commit before force pushes but I can't retrieve them because github doesn't allow fetch orphaned commits. See here. I also can't easily tell which one I want, because i can't view history of an orphaned commit.

I can try adding a local dictionary in a simpler way from there.

[d-torrance]

Sure thing!

https://github.com/d-torrance/M2/tree/test-unary

[mahrud]

Remind me, what is wrong with the version on your test-unary branch?

[d-torrance]

Remind me, what is wrong with the version on your test-unary branch?

No local dictionaries for the tests, e.g., #3490 (comment)

[mahrud]

I think it's going to take me more time than I have before November. Is it okay with you if we postpone this for the next release?

[d-torrance]

I think it's going to take me more time than I have before November. Is it okay with you if we postpone this for the next release?

Yes, that's totally fine!