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.

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

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

Merged
merged 2 commits into from
Sep 3, 2023

Conversation

CFiggers
Copy link
Contributor

@CFiggers CFiggers commented Sep 1, 2023

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
Copy link
Contributor

sogaiu commented Sep 2, 2023

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
Copy link
Contributor Author

CFiggers commented Sep 2, 2023

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
Copy link
Contributor Author

CFiggers commented Sep 2, 2023

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

@sogaiu
Copy link
Contributor

sogaiu commented Sep 2, 2023

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 (^^;

@bakpakin bakpakin merged commit 707e9bc into janet-lang:master Sep 3, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants