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

Add testing spec for int.ts file #164

Merged
merged 9 commits into from
May 26, 2022
Merged

Add testing spec for int.ts file #164

merged 9 commits into from
May 26, 2022

Conversation

MartinMinkov
Copy link
Contributor

Description
Adds initial test spec for all classes in the int.ts file. This includes Int64, UInt64, and UInt32.

There are a number of bugs uncovered while writing unit tests for these classes, some are documented as issues and others are not. I will be looking through our issues list and adding bugs that are not currently present.

The current tests that fail due to bugs are commented out with their error message as a comment. If one wishes to inspect further, uncomment the test and run npm run test

Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow a lot of test cases, nice!

  1. Can you make issues all the failing tests?
  2. Is there some way we can test the circuits too? I think right now we're testing the "outside the circuit" version of the logic. We may need to do some crazy ocaml stuff in order to do constraint checking without proof generation (this will be fast, but still catch errors) -- maybe @mitschabaude already knows if this is exposed?

afterAll(async () => {
setTimeout(async () => {
await shutdown();
}, 1500);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there a long timeout here before we shutdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced this with a timeout of 0. The reason we use a timeout at all is that await shutdown() calls process.exit() under the hood and introduces a race condition with the Jest output and just shutting down the running process. By using a timeout of 0, we can defer the await shutdown until everything else in the tests is done executing and it doesn't cut off the test output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetTimeout 0 as a defer seems okay to me 👍

@mitschabaude
Copy link
Member

Oh wow, so many failure cases! This is great to have.

Is there some way we can test the circuits too? I think right now we're testing the "outside the circuit" version of the logic. We may need to do some crazy ocaml stuff in order to do constraint checking without proof generation (this will be fast, but still catch errors) -- maybe @mitschabaude already knows if this is exposed?

I agree that we'd like to have some in-snark testing as well. There's Circuit.runAndCheck, I don't know in detail what it does but I think it's designed for exactly that

@mitschabaude mitschabaude mentioned this pull request May 23, 2022
Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@mitschabaude mitschabaude merged commit 7e140e7 into main May 26, 2022
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.

None yet

3 participants