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

Simplified regex in removeJsonQuotes #419

Merged
merged 9 commits into from
Nov 11, 2022

Conversation

jdsteinhauser
Copy link

@jdsteinhauser jdsteinhauser commented Sep 17, 2022

Resolves Issue #415. I'm not satisfied with how I had to test it, since Jest wasn't happy with any way I tried to import src/lib/fetch.[jt]s . If an RFC is needed, I'll gladly write one but I was assuming this was small and straightforward enough to not require one.

  • cleaned up the tests
  • added some tests
  • made sure it works with safari

@shimkiv
Copy link
Member

shimkiv commented Sep 18, 2022

Seems to work because now it reveals another issue (no stacktrace available):

image

Copy link
Collaborator

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

Very nice! One more test and I'm happy :D

The web workers thing is a bummer, but we'll manage to work around that as well

src/lib/fetch.test.ts Outdated Show resolved Hide resolved
@mitschabaude
Copy link
Collaborator

mitschabaude commented Sep 18, 2022

@jdsteinhauser re: testing. Our Jest setup currently doesn't manage to build the whole project, so right now with Jest we can only test stuff that is exported from snarkyjs (all the other Jest test import "snarkyjs" which resolves to the build output in /dist/node).

We recently introduced a way to test internal stuff as well -- you can see it demonstrated in src/lib/hash-inputs.unit-test.ts.
-> Just make a new file with the ending unit-test.ts, and it will automatically be built and run as part of npm run test:unit.
You can't use describe and it in these files, but you can import and use expect (which is just a utility function that throws an error if a test fails), so you get the same testing capabilities, just with less fancy console output

@bkase
Copy link
Member

bkase commented Sep 27, 2022

@jsteinhauser could you rebase the PR off of main instead?

@Trivo25 Trivo25 self-assigned this Oct 16, 2022
@Trivo25 Trivo25 changed the base branch from releases to main November 10, 2022 14:56
@Trivo25
Copy link
Member

Trivo25 commented Nov 10, 2022

  • cleaned up the tests
  • added some tests
  • made sure it works with safari

Copy link
Collaborator

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

awesome!

@jdsteinhauser
Copy link
Author

Oh my goodness, thanks for finishing this one out @Trivo25 !

@Trivo25
Copy link
Member

Trivo25 commented Nov 11, 2022

Oh my goodness, thanks for finishing this one out @Trivo25 !

Sorry it took so long :D

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.

5 participants