yetibot / core

:expressionless: Core yetibot utilities, extracted for shared use among Yetibot and its various plugins
https://yetibot.com
Eclipse Public License 1.0
27 stars 17 forks source link

loading of db schema and required commands for test NS #213

Closed gkspranger closed 2 years ago

gkspranger commented 2 years ago

oh-kay, here's the situation (<< name that tune)

i want to work on and test a specific NS, and only that NS .. when running tests for the 1st time in a clean environment (docker-compose up -d 1st run, no data, no previous runs of test), i get fails when ONLY testing that NS .. example ::

lein test yetibot.core.test.parser
...
...
22-04-20 19:40:03 GKS-MBP.local INFO [yetibot.core.interpreter:160] - reduce commands ([:cmd [:words "category" [:space " "] "names"]] [:cmd [:words "echo" [:space " "] "async:" [:space " "] [:sub-expr [:expr [:cmd [:words "render" [:space " "] "{{async}}"]]]]]])

22-04-20 19:40:03 GKS-MBP.local INFO [yetibot.core.db.util:194] - db query ["SELECT * FROM yetibot_channel WHERE chat_source_adapter=? AND chat_source_channel=?" "nil" nil]

FAIL about parse-and-eval - Sub expressions can access the data propagated from the previous pipe at (parser.clj:257)
Expected:
"async: commands that execute asynchronously"
Actual:
org.postgresql.util.PSQLException: ERROR: relation "yetibot_channel" does not exist
  Position: 15
  org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2497)

oh-kay, so DB hasn't been setup yet .. so let me do that ::

you will see me requiring that db NS -- and since the loader function is a defonce -- it will load the schema (yay for idempotent schema loading !!) .. i then proceed with running my tests -- again, only for a specific NS (parser) using lein test .. but then i need to solve the problem of making sure the commands i am testing are in fact loaded -- so i use the load-ns function to do that ..

at the end of it all, i have a "clean" way of mod'ing/testing a single core NS and test NS .. no issues, just the stuff ..

is this OK ?? is my brain not connecting synapsis ?? i get that we should be focusing on repl driven development, and i can do that for the most part -- but when i am thinking of how YB is tested, and issues i have ran into before (yetibot_channel not existing, unknown commands because they haven't been loaded, etc) -- it seems like this is a semi-OK way to making sure what i need prepared is in fact prepared ..

i don't think we have control over the ordering of what midje loads 1st, 2nd, etc .. and while i can't find the doc right this instance -- i am pretty sure i read that midje wants you to think of each fact as completely independent, isolated test units and that you should not depend on any specific loading order of said facts ..

anway -- just testing out some ideas i have about how to move forward with some testing i want to do .. open to any and all ideas ..

thanks !! no rush

codecov[bot] commented 2 years ago

Codecov Report

Merging #213 (261923d) into master (42405be) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #213   +/-   ##
=======================================
  Coverage   62.51%   62.51%           
=======================================
  Files          95       95           
  Lines        4164     4164           
  Branches      235      235           
=======================================
  Hits         2603     2603           
  Misses       1326     1326           
  Partials      235      235           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 42405be...261923d. Read the comment docs.

devth commented 2 years ago

This makes sense to me. Pretty sure you're right about not having control over Midje loading order, and even if we did that doesn't seem like the right solution. With your solution, namespaces just require (or load-ns) the things they need, so tests can be run in any order.

The only thing that comes to mind is I was probably lazy about not cleaning up test data. Probably doesn't matter in most cases but eventually we might run into a case where a test needs to reset things. But for now this lgtm!

devth commented 2 years ago

@gkspranger ready to merge?

gkspranger commented 2 years ago

@gkspranger ready to merge?

IF you are OK with seeing more of these types of PRs -- then yes .. i don't think i made it clear that any future tests i write that require the DB being loaded and/or have a dependency on a specific command(s) -- i would perform the same mechanics you see here << just because i have ran into this issue before and while (load-commands) works, IMO it is overkill for the small unit of work i am trying to focus on ..

devth commented 2 years ago

Yep, makes sense!