Add specs for ns datahike.api #596

jsmassa · 2023-01-11T20:51:40Z

Renewal of PR #330

jsmassa · 2023-01-20T11:00:11Z

test/datahike/test/validation_test.cljc

@@ -30,7 +30,7 @@
      [{:db/id -1 :profile #"regexp"}])

    (is (thrown-with-msg? Throwable #"Unknown operation" (d/db-with db [["aaa" :name "Ivan"]])))
-    (is (thrown-with-msg? Throwable #"Bad entity type at" (d/db-with db [:db/add "aaa" :name "Ivan"])))
+    #_(is (thrown-with-msg? Throwable #"Bad entity type at" (d/db-with db [:db/add "aaa" :name "Ivan"]))) ;; todo: how to test incorrect spec now?


At the moment the specs are being checked when the tests are run, so db-with already fails on the specs and it doesn't even get to the point where it would throw "Bad entity type".

The question is now if we want to to keep on checking the specs for testing or we should be able to have tests like this one here, which is good to have for documentation purposes in order to see what's happening.

But if we were not using the specs in the tests the question would be where and how else we would use them. One option would be in development environments. Probably not production though as is would slow down the code, right?

Yes, I think this is a common way to enforce contracts/spec like that, to activate them in development and debugging and turn them off in production. It really depends though also on the production environment, since they guard an interface to external access here. The main direct benefit of having specs at all in my opinion is to have automatically checked documentation for what the inputs should be.

I think this here is an edge case where we would have two guards. Ideally we could just tell that this spec check should be always on, not sure whether this is possible.

Hm, I can't find any documentation about how it would be possible to automatically turn on instrumentation for development. It looks like it is always turned on manually by calling s/instrument in the repl. There are libraries that are doing it differently, but they might have less features.

I would suggest removing the instrumentation for the tests for now and instead use the specs for generative testing in the future.

Hm, that's another option. I could live with that as well, but I think I would still personally prefer removing the instrumentation from the tests entirely.

I am fine with not instrumenting the specs but then probably no one ever uses them again:) So maybe we should have some kind of interface-test? Just an idea...

What do you mean by 'interface-test'?

As mentioned above, I would like to use them for generative tests which would act like a check if the specs are correct as well as enable us to potentially discover bugs that we wouldn't have discovered otherwise. Is that what you mean as well?

Yes, I'd say that's what I meant. But I would rephrase it a bit... the specs are basically defining our contract with the world and therefore we should keep some kind of test that this contract is not broken and works reliably.

I have created issue #605 for this topic now. Feel free to change the wording or add stuff that you think is missing.

kordano

Looks good, might need another discussion on the dev integration. But then it would be ready.

src/datahike/api.cljc

TimoKramer

Did not go through in detail with the specs. But so far looks good to me. Thanks for finishing this long standing PR @jsmassa! Especially remarkable are the changes in the api-ns where you are using this raise-macro which is awesome, wondering who made this ... 🤔. I should have used this when rewriting the api-ns.

src/datahike/api.cljc

src/datahike/spec.cljc

test/datahike/test/schema_test.cljc

test/datahike/test/time_variance_test.cljc

test/datahike/test/utils.cljc

TimoKramer · 2023-02-14T12:05:25Z

test/datahike/test/validation_test.cljc

@@ -30,7 +30,7 @@
      [{:db/id -1 :profile #"regexp"}])

    (is (thrown-with-msg? Throwable #"Unknown operation" (d/db-with db [["aaa" :name "Ivan"]])))
-    (is (thrown-with-msg? Throwable #"Bad entity type at" (d/db-with db [:db/add "aaa" :name "Ivan"])))
+    #_(is (thrown-with-msg? Throwable #"Bad entity type at" (d/db-with db [:db/add "aaa" :name "Ivan"]))) ;; todo: how to test incorrect spec now?


Did you look into unstrument? Maybe we can stop the instrumentation for certain tests?

TimoKramer · 2023-02-14T12:17:48Z

test/datahike/test/validation_test.cljc

@@ -30,7 +30,7 @@
      [{:db/id -1 :profile #"regexp"}])

    (is (thrown-with-msg? Throwable #"Unknown operation" (d/db-with db [["aaa" :name "Ivan"]])))
-    (is (thrown-with-msg? Throwable #"Bad entity type at" (d/db-with db [:db/add "aaa" :name "Ivan"])))
+    #_(is (thrown-with-msg? Throwable #"Bad entity type at" (d/db-with db [:db/add "aaa" :name "Ivan"]))) ;; todo: how to test incorrect spec now?


(st/unstrument) (is (thrown-with-msg? Throwable #"Bad entity type at" (d/db-with db [:db/add "aaa" :name "Ivan"]))) (st/instrument))

Seems to work. Don't know if this works well because of the concurrency of the tests, though.

src/datahike/spec.cljc

* Add Clojure Specs for the `datahike.api` namespace --------- Co-authored-by: Fabrizio Ferrai <[email protected]>

f-f and others added 6 commits July 28, 2021 15:09

Add Clojure Specs for the datahike.api namespace

51b1a2e

Merge f-f:spec-api-ns and fix

546a6ca

Fix tests

85614a4

Uncomment tests

600e4f8

Minor changes

45954a8

fix formatting

e7d5e7e

jsmassa mentioned this pull request Jan 11, 2023

Add specs for the datahike.api namespace #330

Closed

Merge branch 'main' into spec-api-ns

3526561

jsmassa marked this pull request as ready for review January 11, 2023 21:43

jsmassa self-assigned this Jan 11, 2023

jsmassa requested review from kordano and whilo January 11, 2023 21:44

jsmassa commented Jan 20, 2023

View reviewed changes

kordano reviewed Jan 31, 2023

View reviewed changes

src/datahike/api.cljc Outdated Show resolved Hide resolved

src/datahike/api.cljc Show resolved Hide resolved

src/datahike/api.cljc Show resolved Hide resolved

TimoKramer previously approved these changes Feb 14, 2023

View reviewed changes

Refine specs and clean up

6162bce

jsmassa dismissed TimoKramer’s stale review via 6162bce February 14, 2023 19:11

jsmassa added 3 commits February 14, 2023 21:36

Use spec from config namespace

d19d683

remove predicate spec

2936085

use bb for native-image lib build

97d88f5

TimoKramer previously approved these changes Feb 16, 2023

View reviewed changes

src/datahike/spec.cljc Show resolved Hide resolved

src/datahike/spec.cljc Show resolved Hide resolved

src/datahike/spec.cljc Show resolved Hide resolved

jsmassa mentioned this pull request Feb 20, 2023

Add generative tests at least for API namespace #605

Open

Remove instrumentation from tests

7a34d52

jsmassa dismissed TimoKramer’s stale review via 7a34d52 February 20, 2023 18:29

TimoKramer self-requested a review February 23, 2023 15:34

TimoKramer approved these changes Feb 23, 2023

View reviewed changes

jsmassa merged commit 3625234 into main Feb 23, 2023

jsmassa deleted the spec-api-ns branch February 23, 2023 15:40

jsmassa mentioned this pull request Feb 27, 2023

test: spec profile #606

Merged

whilo pushed a commit that referenced this pull request Mar 5, 2023

Add specs for ns datahike.api (#596)

38a1b53

* Add Clojure Specs for the `datahike.api` namespace --------- Co-authored-by: Fabrizio Ferrai <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add specs for ns datahike.api #596

Add specs for ns datahike.api #596

jsmassa commented Jan 11, 2023

jsmassa Jan 20, 2023

jsmassa Jan 20, 2023

whilo Jan 21, 2023

whilo Jan 21, 2023

jsmassa Jan 26, 2023 •

edited

Loading

jsmassa Feb 14, 2023

TimoKramer Feb 15, 2023

jsmassa Feb 15, 2023

TimoKramer Feb 16, 2023

jsmassa Feb 20, 2023

kordano left a comment

TimoKramer left a comment

TimoKramer Feb 14, 2023

TimoKramer Feb 14, 2023

Add specs for ns datahike.api #596

Add specs for ns datahike.api #596

Conversation

jsmassa commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsmassa Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kordano left a comment

Choose a reason for hiding this comment

TimoKramer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsmassa Jan 26, 2023 •

edited

Loading