janet-lang / spork

Various Janet utility modules - the official "Contrib" library.
MIT License
119 stars 35 forks source link

Code quality improvements, bugfixes, additional tests for `spork/data` #153

Closed CFiggers closed 1 year ago

CFiggers commented 1 year ago

Added additional tests to /test/suite0021.janet , applying to spork/data/diff.

This revealed bugs in the way diff was handling larger, more deeply-nested structures.

While fixing bugs, simplified diff's logic which allowed eliminating several small "helper" functions. The eliminated function calls probably improve performance of diff, but I haven't benchmarked before/after.

All tests, including newly-added ones, now pass.

sogaiu commented 1 year ago

All tests, including newly-added ones, now pass.

Yay!

Some of the later tests make my head spin :)


May be minor point but I think start-suite's argument is currently optional. May be it's not necessary to add one in this case?

I tried running the tests with and without an argument and the output didn't seem different wrt the presence of this argument -- though I might have missed something (^^;

When reorganizing janet's tests, we made a similar change and I think it made it a bit easier for maintenance. Possibly a similar thing holds here.

CFiggers commented 1 year ago

I was just making this one match all the others that are currently out there. I noticed it because the output from jpm test is inconsistent between all the other test suites and suite0021:

image

I don't really have an opinion either way on which way is preferable. ¯\_(ツ)_/¯

CFiggers commented 1 year ago

...Of course, now I notice that there's already tests out there organized by module instead of just being opaquely numbered. 😜

sogaiu commented 1 year ago

I noticed it because the output from jpm test is inconsistent between all the other test suites and suite0021

Ah right -- I guess I did miss the difference :)

there's already tests out there organized by module instead of just being opaquely numbered. 😜

I don't know if it would be seen as worth it, but I suppose we might be able to reorganize in a manner somewhat similar to what we did for Janet itself.

Though may be things are already gradually heading in that direction and it's another thing I failed to notice (^^;