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

Update testing macro to update-current-env #48

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

jaidetree
Copy link
Contributor

Previously, the testing macro would update a testing-contexts dynamic var, but the APIs around for stringifying testing-context would read from the current-env.

Given the official cljs impl updates the current-env, this PR applies the same approach.

@borkdude
Copy link
Collaborator

Just to double-check, did you test this with nbb locally?

@jaidetree
Copy link
Contributor Author

Just to double-check, did you test this with nbb locally?

I did not yet. What's that process look like? Clone nbb, update deps to point to local sci.configs, build nbb, and link to the artifact in my dev repo with the tests?

@borkdude
Copy link
Collaborator

yes

@borkdude
Copy link
Collaborator

what you could also do is make a fork of sci.configs and then make a candidate PR for nbb based on your fork of sci.configs which tests the change, and after that, we merge it into sci.configs

@jaidetree
Copy link
Contributor Author

I'll give that a shot. Also looks like my formatter added more noise than desired. Going to try again first to mitigate that.

@jaidetree jaidetree force-pushed the fix-testing branch 4 times, most recently from ae15a86 to a60b46b Compare June 25, 2024 15:50
Previously, the testing macro would update a testing-contexts dynamic
var, but the APIs around for stringifying testing-context would read from
the current-env.

Given the official cljs impl updates the current-env, this PR applies the
same approach.
@jaidetree
Copy link
Contributor Author

jaidetree commented Jun 25, 2024

Created babashka/nbb#359 as my best attempt at what you suggested for testing. Also did test it locally:

Testing jaide.test

FAIL in (simple-test) (/Users/j/projects/nbb/test.cljs:10)
If you see this the testing macro is working
expected: (= (+ 1 1) 3)
  actual: (not (= 2 3))

Ran 1 tests containing 1 assertions.
1 failures, 0 errors.

@borkdude borkdude merged commit ccd6624 into babashka:main Jun 25, 2024
3 checks passed
@borkdude
Copy link
Collaborator

I guess now you can update your nbb PR to use the newest sci.configs and then we'll merge that

@jaidetree
Copy link
Contributor Author

I guess now you can update your nbb PR to use the newest sci.configs and then we'll merge that

Done. Thanks so much!

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

2 participants