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.

Sign up for GitHub

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

Jump to bottom

Support testing inside checking and make checking options optional #65

Merged
merged 2 commits into from May 15, 2021
Merged

Support testing inside checking and make checking options optional #65

merged 2 commits into from May 15, 2021

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Mar 23, 2021 •
edited

Close #57

This PR adds support for nesting testing inside checking forms and makes the options argument to checking optional.

@frenchy64 frenchy64 changed the title checking: support ct/testing and optional qc map Support testing inside checking and make checking options optional Mar 23, 2021
@frenchy64 frenchy64 force-pushed the frenchy64-checking-with-testing branch from 4dbf2af to 2a8b5fa Compare March 23, 2021 20:51
@frenchy64 frenchy64 marked this pull request as ready for review March 23, 2021 20:53
@frenchy64 frenchy64 marked this pull request as draft March 23, 2021 21:01
@frenchy64 frenchy64 marked this pull request as ready for review March 23, 2021 21:03
@frenchy64 frenchy64 mentioned this pull request Mar 23, 2021
Merged
gfredericks
gfredericks reviewed Apr 3, 2021
View reviewed changes
Copy link
Owner

@gfredericks gfredericks left a comment

Choose a reason for hiding this comment

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

I didn't write this code and don't have enough of the clojure.test internals in my head to be able to easily judge if the approach makes sense; if you're confident it does and that it's not a breaking change for users, then I don't mind merging it

src/com/gfredericks/test/chuck/clojure_test.cljc Show resolved Hide resolved
src/com/gfredericks/test/chuck/clojure_test.cljc Show resolved Hide resolved
src/com/gfredericks/test/chuck/clojure_test.cljc Show resolved Hide resolved
src/com/gfredericks/test/chuck/clojure_test.cljc Show resolved Hide resolved
@frenchy64
Copy link
Contributor Author

I'll give it another look next week and confirm whether it's breaking. I was pretty happy with it when I wrote it.

@frenchy64
Copy link
Contributor Author

Nothing has come to mind that might be breaking. After another read, I'm pretty sure we're just (carefully) adding logging information. The try/finally in -report seems the most error prone, but I think it's ok. Does it make sense to you?

gfredericks
gfredericks reviewed Apr 18, 2021
View reviewed changes
src/com/gfredericks/test/chuck/clojure_test.cljc
(f))

:cljs
(binding [*chuck-captured-reports* reports-atom
cljs.test/*current-env* (cljs.test/empty-env ::chuck-capture)]
ct/*current-env* (assoc (ct/empty-env ::chuck-capture)
:testing-contexts (:testing-contexts (ct/get-current-env)))]
Copy link
Owner

Choose a reason for hiding this comment

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

is one of these keywords supposed to be namespaced? I'm comparing to the (new) line 112 in particular

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I don't think so.

This forwards the current :testing-contexts to (new) lines 50, 55, 60, 65 so then we can attach them to the report via ::testing-contexts. Then, ::testing-contexts is retrieved in -report.

The same idea is done for :clj on line 65.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clearer, these cannot be namespaced because it's working with envs from cljs.test, which uses :testing-contexts as a field.

gfredericks
gfredericks approved these changes Apr 18, 2021
View reviewed changes
Copy link
Owner

@gfredericks gfredericks left a comment

Choose a reason for hiding this comment

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

Left a single comment/question; I'll merge and release assuming the answer isn't prohibitive.

thanks!

@gfredericks
Copy link
Owner

Thanks!

@gfredericks gfredericks merged commit fe01dd5 into gfredericks:master May 15, 2021
@gfredericks
Copy link
Owner

Released as 0.2.11; thanks again

@frenchy64
Copy link
Contributor Author

Thanks for the feedback!

frenchy64 added a commit to threatgrid/ctia that referenced this pull request May 18, 2021
@frenchy64
Use new test.chuck logging features (#1103)
49e8b20 
<!-- Specify linked issues and REMOVE THE UNUSED LINES -->

test.chuck now supports `testing` within `checking`: gfredericks/test.chuck#65.

<!--

Describe your PR for reviewers.
Don't forget to set correct labels (User Facing / Beta / Feature Flag)
If there is UI change please add a screen capture.
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="iroh-services-clients">[§](#iroh-services-clients)</a> IROH Services Clients
=====================================================================================

Put all informations that need to be communicated to IROH Services Clients.
Typically IROH-UI, ATS Integration, Orbital, etc...
 -->

<a name="qa">[§](#qa)</a> QA
============================

<!--
Describe the steps to test your PR.

1.
2.
3.

Or if no QA is needed keep it as is.
 -->
No QA is needed.

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="ops">[§](#ops)</a> Ops
===============================

  Specify Ops related issues and documentation
- Config change needed: threatgrid/tenzin#
- Migration needed: threatgrid/tenzin#
- How to enable/disable that feature: (ex remove service from `bootstrap.cfg`, add scope to org)
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="documentation">[§](#documentation)</a> Documentation
=============================================================

  Public Facing documentation section;
- Public documentation updated needed: threatgrid/iroh-ui#
  See internal [doc file](./services/iroh-auth/doc/public-doc.org)
 -->

<a name="release-notes">[§](#release-notes)</a> Release Notes
=============================================================

<!-- REMOVE UNUSED LINES -->

```
intern: Use new test.chuck logging features
```

<a name="squashed-commits">[§](#squashed-commits)</a> Squashed Commits
======================================================================
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfredericks gfredericks gfredericks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optional num-tests-or-opts for checking
2 participants
@frenchy64 @gfredericks