Use a list instead of a hash table for storing package tests #3419

d-torrance · 2024-08-20T16:43:41Z

This way, tests will work like other similar functions like methods and about.

As suggested by @mahrud in #3418 (comment).

Before

i1 : tests "FirstPackage"

o1 = HashTable{0 => TestInput[/usr/share/Macaulay2/FirstPackage.m2:54:1-57:1]}

o1 : HashTable

After

i1 : tests "FirstPackage"

o1 = {0 => (TestInput[/home/dtorrance/src/macaulay2/M2/M2/Macaulay2/packages/FirstPackage.m2:54:1-57:1])}

o1 : NumberedVerticalList

Another neat consequence that we get for free is negative indices work:

i2 : tests(-1, "FirstPackage")

o2 = TestInput[/home/dtorrance/src/macaulay2/M2/M2/Macaulay2/packages/FirstPackage.m2:54:1-57:1]

o2 : TestInput

mahrud

Why doesn't code tests Saturation work?

The only other requested change is the one in the documentation, but here are several other desiderata:

The printing of TestInput could use simplifying:
- Why is it in parenthesis inside NumberedVerticalList?
- The full path to the test seems unnecessary, can we cap it to path from packages directory? e.g. TestInput[FirstPackage.m2:54:1-57:1]. Full location can be queried with locate N or locate tests Saturation.
I'd expect capture tests Saturation to work.
Why are code first tests Saturation and code locate first tests Saturation are so different? I understand that the former is the string that we will eventually run, but even the location line is different, and the extra lines before and after look weird. Maybe toString TestInput should give the actual string so code can just show the exact lines in the file?

i5 : code locate first tests Saturation

o5 = ../../Macaulay2/packages/Saturation/quotient-test.m2:1:1-11:1: --source code:
     TEST ///
       R  = ZZ[x, y];
       I  = ideal(6*x^3, 9*x*y, 8*y^2);
       J1 = ideal(-3, x^2);
       J2 = ideal(4*y);
       assert(intersect(I:J1, I:J2) == ideal(8*y^2, 3*x*y, x*y^2, 6*x^3))
       assert(I : (J1+J2) == ideal(8*y^2, 3*x*y, 6*x^3))
       for strategy in {Quotient, Iterate} do
       assert(quotient(I, J1, Strategy => strategy) == ideal(8*y^2, 3*x*y, x*y^2, 6*x^3))
     ///


i6 : code first tests Saturation

o6 =  -- /home/mahrud/Projects/M2/quickfix/M2/Macaulay2/packages/Saturation/quotient-test.m2:11:1: location of test code

       R  = ZZ[x, y];
       I  = ideal(6*x^3, 9*x*y, 8*y^2);
       J1 = ideal(-3, x^2);
       J2 = ideal(4*y);
       assert(intersect(I:J1, I:J2) == ideal(8*y^2, 3*x*y, x*y^2, 6*x^3))
       assert(I : (J1+J2) == ideal(8*y^2, 3*x*y, 6*x^3))
       for strategy in {Quotient, Iterate} do
       assert(quotient(I, J1, Strategy => strategy) == ideal(8*y^2, 3*x*y, x*y^2, 6*x^3))


i7 :

M2/Macaulay2/packages/Macaulay2Doc/functions/tests-doc.m2

d-torrance · 2024-08-20T23:50:02Z

Why doesn't code tests Saturation work?

It works now!

The only other requested change is the one in the documentation, but here are several other desiderata:

The printing of TestInput could use simplifying:

Why is it in parenthesis inside NumberedVerticalList?

That was because precedence(TestInput) wasn't defined. Adding that removed the parentheses!

The full path to the test seems unnecessary, can we cap it to path from packages directory? e.g. TestInput[FirstPackage.m2:54:1-57:1]. Full location can be queried with locate N or locate tests Saturation.

I'd rather stick with the current behavior for Emacs clickability.

I'd expect capture tests Saturation to work.

I've added capture(TestInput), so one test can now be captured at a time. capture(List) expects a list of strings, so it would need to be rewritten to allow TestInput objects.

Why are code first tests Saturation and code locate first tests Saturation are so different? I understand that the former is the string that we will eventually run, but even the location line is different, and the extra lines before and after look weird. Maybe toString TestInput should give the actual string so code can just show the exact lines in the file?

toString(TestInput) now works as you suggested, and code(TestInput) now returns a DIV like all the other code methods (it's just code @@ locate now).

mahrud · 2024-08-21T00:29:00Z

M2/Macaulay2/m2/testing.m2

-        (minimizeFilename currentFileName, currentRowNumber(), teststring);
-    currentPackage#"test number" = n + 1;)
+    n := #currentPackage#"test inputs";
+    if opts.FileName then (


Is this option ever used? I have seen TEST get "test.m2" but never this option. Wouldn't be opposed to deprecating it for simplicity.

Oooh actually this seems like a bug:

i1 : TEST get "../../Macaulay2/packages/Macaulay2Doc/demo1.m2" i2 : tests User o2 = {0 => TestInput[stdio:-13:1-1:80]} i3 : locate oo o3 = {0 => (stdio:-13:1-1:80)} i4 : code oo o4 = stdio:-13:1-1:80: --source code: -- This is the beginning of your Macaulay2 log stored at /home/mahrud/.Macaulay2/history.m2

Sorry to be making this PR more complicated for you!

The FileName option is used for all the Core tests. JSON too. I'm not sure if there are any others -- it's relatively new.

Hmm, yeah that's a tricky bug. I think it's probably always existed for locate, since it assumes we're calling TEST on a literal string and not the output of some function. It's just now that code is calling locate, it's showing up there, too.

I guess the documentation for TEST could discourage the use of get for this reason, and we could modify the handful of tests that currently use it. (I'm only counting 13.)

Let's first try to find a way to fix the bug because I'd rather keep allowing TEST get ..., it's much simpler than using an option.

mahrud · 2024-08-21T14:27:21Z

I'd rather stick with the current behavior for Emacs clickability.

Can you say more about how you use this? e.g. I would imagine locate tests Saturation would still produce a clickable list. I think eventually it would be good if the printing of a TestInput looked something closer to a document tag, telling you that this is a test for such and such method.

d-torrance · 2024-08-21T16:22:06Z

My initial rationale for introducing tests was for debugging failing builds. We'd have some log that said such and such test failed, but it was non-trivial to figure out what exactly that test was and where it was located in the source. I figured using something similar to net(Function) would be useful. A test isn't a function, but it's kind of similar in that they both consist of some hunk of code.

So now, if we see on some log that check(37, "Foo") failed, we can just run tests "Foo" and click on the 37 => TestInput[...] link that shows up to start debugging.

This way, "tests" will work like other similar functions like "methods" and "about".

Avoids unnecessary parentheses in output of "tests".

Rather than storing the filename and line number separately, we store a FilePosition containing both of them. We update locate(TestInput) to use this. We also remove the redundant source file stamp from the code -- there's no reason to also store the file location here. Finally, we remove the NewFromMethod for constructing a TestInput from a sequence. There was really no reason to use this, as we only construct a TestInput object once, inside TEST.

Now returns source code of test. Update an error message when "check" is called with the Verbose option that previously used the old toString behavior.

Now returns a DIV like other "code" methods.

Now that the test inputs are stored in a list, we can just use the length of the list.

d-torrance · 2024-08-31T05:46:23Z

I've fixed the test location bug, which required turning TEST into a keyword so that we could figure out the location of its argument while it's still an unevaluated Code object.

I also added the ability for TEST to accept a File object. This way, the location is actually the source of the test rather than the line that calls get. For example:

i1 : TEST get "/usr/share/Macaulay2/Macaulay2Doc/demo1.m2"

i2 : TEST openIn "/usr/share/Macaulay2/Macaulay2Doc/demo1.m2"

i3 : tests User

o3 = {0 => TestInput[stdio:1:5-1:53]                                     }
     {1 => TestInput[/usr/share/Macaulay2/Macaulay2Doc/demo1.m2:1:1-14:9]}

o3 : NumberedVerticalList

i4 : code oo

o4 = stdio:1:5-1:53: --source code:
     TEST get "/usr/share/Macaulay2/Macaulay2Doc/demo1.m2"
     ---------------------------------------------------------------------------
     /usr/share/Macaulay2/Macaulay2Doc/demo1.m2:1:1-14:9: --source code:
     -- when running examples, we usually specify the --stop option
     -- here we reverse that so we can demonstrate the debugger in an example
     stopIfError = false
     debuggingMode = true

     f := x -> (
          a := "hi there";
          b := 1/x;
          b+1)

     g = y -> (
          c := f(y-1);
          d := f(y-2);
          c+d)

mahrud · 2024-08-31T09:09:53Z

I also added the ability for TEST to accept a File object. This way, the location is actually the source of the test rather than the line that calls get.

In principle this sounds useful, but I can totally imagine it becoming a pain to figure out where a test file was loaded from!

d-torrance · 2024-08-31T11:13:24Z

In principle this sounds useful, but I can totally imagine it becoming a pain to figure out where a test file was loaded from!

Great point! I'll remove the File method.

mahrud · 2024-08-31T12:45:41Z

By the way, one benefit of turning TEST into a keyword is that the content in front of it doesn't have to be a string anymore. It could literally be TEST ( ... ) and the stuff in front could be kept for future evaluation.

Not necessarily suggesting doing this now, but worth considering.

d-torrance · 2024-08-31T18:33:20Z

M2/Macaulay2/packages/LLLBases.m2

@@ -1248,6 +1248,7 @@ TEST ///

 TEST 
 /// -- DON'T TEST YET??
+    -- no-check-flag (encountered an unknown key or option: Engine)


@mikestillman - This test appears to never have been run since TEST and the test string were on separate lines. After changing TEST to a keyword, the test does get run, but it fails because LLL doesn't have an Engine option.

I'm proposing that we continue skipping it for now (which no-check-flag takes care of), but I figured I'd tag you in case you'd like to take a look.

Was there an engine implementation that got removed or never got added? Seems strange.

M2/Macaulay2/d/binding.d

M2/Macaulay2/tests/normal/names.m2

M2/Macaulay2/d/common.d

M2/Macaulay2/m2/testing.m2

mahrud · 2024-08-31T20:47:16Z

M2/Macaulay2/packages/JSON.m2

-
-TEST(currentPackage#"source directory" | "JSON/test-encode.m2",
-    FileName => true)
+TEST get(currentPackage#"source directory" | "JSON/test-parse.m2")


I think just TEST "./JSON/..." should always work, no?

It doesn't :( I get the following:

i1 : check "JSON" ../src/macaulay2/M2/M2/Macaulay2/packages/JSON.m2:361:0:(2):[18]: error: opening input file "./JSON/test-parse.m2" failed: No such file or directory

I think it depends on the value of currentDirectory(). If I changeDirectory into the packages directory, then it works just fine.

Hmm but load "./..." works correctly. What's the difference? Does TEST(currentFileDirectory | ...) work?

Yes, it works with currentFileDirectory

Ah, the difference is that load looks everywhere under path but get doesn't. A little strange to be honest.

M2/Macaulay2/packages/Macaulay2Doc/functions/check-doc.m2

M2/Macaulay2/d/common.d

mahrud · 2024-09-01T02:46:31Z

I'm hesitant about the broader interpreter changes in this PR. Could you scale back this PR to just the new TEST related changes?

mahrud · 2024-09-01T02:51:51Z

M2/Macaulay2/packages/JSON.m2

-
-TEST(currentPackage#"source directory" | "JSON/test-encode.m2",
-    FileName => true)
+TEST get(currentPackage#"auxiliary files" | "test-parse.m2")


Reading this line, I would be more confused about where to look for that file than before. You can of course go with whatever solution you prefer in your package.

This way, we can access its argument while it's still a Code object and properly determine its location rather than guessing.

check(2, "LLLBases") wasn't previously being registered since TEST and the test string were on different lines. Now it's picked up, but it raises an error (unknown key: Engine), so we skip it

mahrud · 2024-09-01T09:52:29Z

Hmm one last thing that comes to mind: is there an easy way to make sure none of the undistributed-packages stop working because of this change to TEST? I doubt it's the case, but it's possible. Maybe we should also run the tests / examples of those packages on GitHub.

d-torrance · 2024-09-01T12:15:13Z

I checked the undistributed packages locally on this branch. A good chunk of them don't even load -- perhaps why they're undistributed -- but the tests pass for the rest of them:

 -- warning: CacheFileName has no tests
 -- warning: CodepthThree did not load
 -- warning: CustomEngineTests did not load
 -- capturing check(0, "ExampleFreeResolutions") -- .102555s elapsed
 -- capturing check(1, "ExampleFreeResolutions") -- .0819185s elapsed
 -- capturing check(2, "ExampleFreeResolutions") -- .105076s elapsed
 -- running   check(3, "ExampleFreeResolutions") -- 1.68029s elapsed
 -- warning: ExampleIdeals has no tests
 -- warning: FastLinearAlgebra did not load
 -- warning: FreeResolutions has no tests
 -- warning: FrobeniusMultiplicities did not load
 -- warning: Functoriality did not load
 -- capturing check(0, "GenerateD")              -- .0782202s elapsed
 -- capturing check(1, "GenerateD")              -- .078785s elapsed
 -- capturing check(2, "GenerateD")              -- .082572s elapsed
 -- capturing check(3, "GenerateD")              -- .0757686s elapsed
 -- capturing check(4, "GenerateD")              -- .0796765s elapsed
 -- running   check(5, "GenerateD")              -- 3.36057s elapsed
 -- capturing check(6, "GenerateD")              -- .0784762s elapsed
 -- warning: MemoryLeaks did not load
 -- capturing check(0, "MGBInterface")           -- .114109s elapsed
 -- capturing check(1, "MGBInterface")           -- .091242s elapsed
 -- running   check(2, "MGBInterface")           -- 3.44805s elapsed
 -- running   check(3, "MGBInterface")           -- 1.77514s elapsed
 -- running   check(4, "MGBInterface")           -- 1.74921s elapsed
 -- running   check(5, "MGBInterface")           -- 1.79347s elapsed
 -- running   check(6, "MGBInterface")           -- 4.34543s elapsed
 -- warning: Mysql did not load
 -- capturing check(0, "NAGtools")               -- 1.14616s elapsed
 -- warning: ParameterSchemes did not load
 -- warning: Polyhedra2 did not load
 -- warning: PolyhedralObjects did not load
 -- warning: PolymakeInterface did not load
 -- warning: RandomSearch has no tests
 -- warning: RisaAsir did not load
 -- warning: Sage did not load
 -- warning: SchurRingsOld has no tests
 -- warning: SecondPackage did not load
 -- warning: SuffixTrees has no tests
 -- warning: ToricCohomology did not load

mahrud · 2024-09-01T13:57:39Z

A good chunk of them don't even load -- perhaps why they're undistributed

This is a matter for another time, but this isn't great. If the undistributed packages don't load, what's the point of keeping them?

mahrud · 2024-10-20T23:22:26Z

M2/Macaulay2/tests/normal/testing.m2

-assert Equation(code pkgtest, expectedCode)
-assert Equation(code 0, expectedCode)
-
+assert Equation(toSequence locate pkgtest, (testpkg, 3, 5, 3, 32, 3, 5))


This happened in a brew build:

-- running check(357, "Core") -- .380111s elapsed /opt/homebrew/Cellar/macaulay2/1.24.11-rc1/share/doc/Macaulay2/Core/tests/testing.m2:1:1 error: -- i5 : pkgtest = tests(0, "TestPackage") -- -- o5 = TestInput[../../M2-93313-0/0.m2:3:5-3:32] -- -- o5 : TestInput -- -- i6 : assert instance(pkgtest, TestInput) -- -- i7 : assert Equation(toSequence locate pkgtest, (testpkg, 3, 5, 3, 32, 3, 5)) -- stdio:10:6:(3): error: assertion failed: -- (../../M2-93313-0/0.m2, 3, 5, 3, 32, 3, 5) == (/private/tmp/M2-93313-0/0.m2, 3, 5, 3, 32, 3, 5) is false -- stdio:1:5:(3): error: test(s) #333 of package Core failed.

Oh, that makes sense. Calling minimizeFilename should fix it. I'll open a PR.

d-torrance requested a review from mahrud August 20, 2024 16:43

mahrud requested changes Aug 20, 2024

View reviewed changes

M2/Macaulay2/packages/Macaulay2Doc/functions/tests-doc.m2 Outdated Show resolved Hide resolved

d-torrance force-pushed the test-list branch from 99034ea to 3042d85 Compare August 20, 2024 23:40

d-torrance force-pushed the test-list branch from 3042d85 to 821a7d6 Compare August 21, 2024 00:10

mahrud reviewed Aug 21, 2024

View reviewed changes

d-torrance marked this pull request as draft August 29, 2024 16:47

d-torrance added 9 commits August 31, 2024 00:30

Use a list instead of a hash table for storing package tests

89110d0

This way, "tests" will work like other similar functions like "methods" and "about".

Update "tests" docs now that a numbered vertical list returned

85b63be

Use net(Function) for net(TestInput)

ff85e40

Add precedence(TestInput)

aa978d1

Avoids unnecessary parentheses in output of "tests".

Change behavior of toString(TestInput)

ed6bf3e

Now returns source code of test. Update an error message when "check" is called with the Verbose option that previously used the old toString behavior.

Update code(TestInput)

21f73cc

Now returns a DIV like other "code" methods.

Drop unnecessary "test number" key from packages

b0d2fc3

Now that the test inputs are stored in a list, we can just use the length of the list.

Add capture(TestInput)

57530ba

d-torrance force-pushed the test-list branch 2 times, most recently from 2189d5c to 314ae63 Compare August 31, 2024 05:39

d-torrance force-pushed the test-list branch from 314ae63 to 2b97e11 Compare August 31, 2024 05:52

d-torrance force-pushed the test-list branch 2 times, most recently from 7540242 to 234900b Compare August 31, 2024 11:29

d-torrance commented Aug 31, 2024

View reviewed changes