Tests for Stencil ( R←(f⌺g)Y)

[asherbhs]

Description

Add tests for ⌺, both general and special cases. You will need a recent build of the trunk for these tests to run without failures.

PR Checklist (Remove options that are not relevant)

• This PR adds tests for a new primitive
• Code coverage has been checked
• Documentation complete (Decision docs, code comments, etc.)

[sloorush]

Hi @asherbhs! Thank you for the PR. 💯

The code coverage document lives on the wiki (for obvious reasons). You can find it here: https://github.com/Dyalog/ullu/blob/main/docs/code-coverage.md

[sloorush]

@asherbhs, I don't think I will have time this week to look at the PR in detail and I am on holiday next week. Are you in a rush to get this reviewed?

[asherbhs]

no worries, I'm not in a hurry to get it merged. I might even get a chance to add even more tests!

[pmikkelsen]

Hi @asherbhs I had a quick look and it looks great over all. I will take a more in-depth look later as well. One thing I noticed is that the entire test runs with ⎕IO set to 0, which means we don't test the cases where ⎕IO is 1. It is of course OK that your model has a fixed IO.

Perhaps something similar happens for some of the other system variables. Do you think you could wrap the entire test in a loop, or something similar, so that your test cases run with both values of ⎕IO (and other system variables where it makes sense)? I know it sometimes seems like it doesn't matter, because your ⎕IO 0 code might hit all the interesting cases already, but all the other ullu tests generally try to cover all combinations of ⎕IO,⎕CT,⎕FR and so on, just to get more coverage. Who knows, perhaps there is a bug in stencil that only shows itself once ⎕IO is 1 and ⎕CT is 0 for example :)

[pmikkelsen]

Actually @asherbhs @sloorush I don't think we usually test with different values of ⎕ML, but when primitives are implemented using MAGIC, couldn't we theoretically run into a bug where the magic code depends on a specific ⎕ML value?

[bear8642]

when primitives are implemented using MAGIC, couldn't we theoretically run into a bug where the magic code depends on a specific ⎕ML value?

And indeed we have! See Mantis 19799 for example

[asherbhs]

Thanks @pmikkelsen and @bear8642, all ⎕FR settings are covered, but we could cover more ⎕CT values in the general case (⎕CT has to be 0 to hit some of the special cases), and we should definitely test with all values of ⎕IO and ⎕ML. I'll get those added before this is merged

[sloorush]

Thanks @asherbhs. I had a quick look at the PR. Looks very good overall 🎊

Two suggestions:

• Can you make the code more commented out, mainly, describing what the tradfns are about so someone reading through the tests gets an idea of where what is fairly quickly.
• Can you run the file through a formatter if not already done. It gets the file in a standard format and helps with later commits to not have formatting diffs. Also, makes the code more palatable for a wider audience because people are used to patterns.

I will spend some more time on this and try to run the tests soon. Is there anything specific that you would like me to have a look at?

[asherbhs]

Thanks @sloorush, I've made those formatting and comment improvements. Let me know if there's anything else I ought to change

[pmikkelsen]

All looks good, except for the one comment which I have added 👍

[sloorush]

Thanks @asherbhs! Did you have a go at checking code coverage with the coverage build and mk_cov?