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

Unit Testing #106

Merged
merged 12 commits into from
Sep 11, 2021
Merged

Unit Testing #106

merged 12 commits into from
Sep 11, 2021

Conversation

jaidetree
Copy link
Collaborator

@jaidetree jaidetree commented Aug 28, 2021

When working on #72 for advising, it became apparent how important testing would be to get that feature in a shippable state.

I drafted a basic unit testing suite to run fennel test files against the hs CLI. It's primitive, but still pretty useful for confirming functionality and behaviors.

image

Docs are included in ./docs/testing.org, with a basic recipe for auto-running tests

@jaidetree jaidetree mentioned this pull request Aug 28, 2021
core.fnl Outdated Show resolved Hide resolved
docs/testing.org Outdated Show resolved Hide resolved
docs/testing.org Outdated Show resolved Hide resolved
docs/testing.org Outdated Show resolved Hide resolved
docs/testing.org Outdated Show resolved Hide resolved
docs/testing.org Outdated Show resolved Hide resolved
docs/testing.org Outdated Show resolved Hide resolved
lib/testing/test.lua Outdated Show resolved Hide resolved
test/statemachine-test.fnl Outdated Show resolved Hide resolved
Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

This is awesome.

core.fnl Outdated Show resolved Hide resolved
@jaidetree
Copy link
Collaborator Author

Made a small change to move pprint into lib/globals.fnl instead of copying the impl twice in the testing/test-runner.fnl and core.fnl

@jaidetree
Copy link
Collaborator Author

jaidetree commented Sep 7, 2021

@agzam Do you have a script to add the license\credits to each file or was that added manually?

Grazfather
Grazfather previously approved these changes Sep 10, 2021
Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

A few nitpicks, looks great over all.

docs/testing.org Outdated
The testing library provides basic unit-testing capabilities to Spacehammer by
running scripts against the hammerspoon CLI =hs=.

Tests can be ran invoking the following shell command within the =~/.hammerspoon= directory:
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be run

docs/testing.org Outdated

*** Does =before= run before each =it=?

Before runs before all =it= calls within a describe block. If you wanted
Copy link
Collaborator

Choose a reason for hiding this comment

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

for extra clarity, something like: Before runs once before all it calls within a..

docs/testing.org Outdated

* Todo List

- [ ] Consider replacing describe and it with =testing= and =is= macros
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should file tickets for these, and we can link.

@@ -237,6 +243,7 @@
:eq? eq?
:filter filter
:find find
:first first
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can turn this table into the shorthand : first version since you're touching it anyway.


(collect-tests)
(run-all-tests)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

hanging

lib/testing/init.fnl Show resolved Hide resolved
lib/testing/test-runner.fnl Show resolved Hide resolved
Grazfather
Grazfather previously approved these changes Sep 11, 2021
Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

Looks good. A few times you use is.eq instead of is.eq? in docs.

docs/testing.org Outdated

(it "Subtraction"
(fn []
(is.eq (- 1 1) 0 "Did not result in 0")))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

is.eq? vs is.eq

docs/testing.org Outdated Show resolved Hide resolved
docs/testing.org Outdated Show resolved Hide resolved
Co-authored-by: Grazfather <grazfather@gmail.com>
Co-authored-by: Grazfather <grazfather@gmail.com>
@jaidetree jaidetree merged commit 02f7bcf into agzam:master Sep 11, 2021
jaidetree added a commit to jaidetree/spacehammer that referenced this pull request Sep 12, 2021
agzam#106

Removed unescaped chars from windows.fnl docstr

Draft of testing libs

Added testing docs and some initial tests

Moved test/testing.org to docs/testing.org

Fixed PR review comments

Removed docstr test code from core.fnl

Moved pprint into lib/globals.fnl

Testing PR review changes from Graz

Update docs/testing.org

Co-authored-by: Grazfather <grazfather@gmail.com>

Update docs/testing.org

Co-authored-by: Grazfather <grazfather@gmail.com>
@agzam
Copy link
Owner

agzam commented Sep 12, 2021

@agzam Do you have a script to add the license\credits to each file or was that added manually?

No, I don't have a script. Btw, would you like to discuss the licensing model, etc.? I think the file headers need a revamp. Maybe we should remove Author: Ag Ibragimov and should keep the list of contributors somewhere else? After so many additions and code modifications, that info in each file doesn't stay relevant for too long.

@agzam
Copy link
Owner

agzam commented Sep 12, 2021

Great work @eccentric-j! This is quite nice. Now, what do you think, would it be very difficult to create a GitHub action and get them run before merging a PR?

@jaidetree
Copy link
Collaborator Author

jaidetree commented Sep 12, 2021

Great work @eccentric-j! This is quite nice. Now, what do you think, would it be very difficult to create a GitHub action and get them run before merging a PR?

Thanks! @Grazfather asked a similar question the other day. That's definitely on my radar, touched on it a bit in #116.

Options to make this work include:

  1. Having the container that the GH action use install OS X + Hammerspoon + Spacehammer and have it run tests similar to local
  2. Mock the hs.* api and run the tests as a regular fennel script (currently it runs through the hs ipc API)

Option #1 is my first choice given that it would run the full tests against hs API which does change from time to time which would be the most accurate.

Option #2 would fix the performance issues, test consistency issues (at least during local development), and the state resetting issues (handled manually in the test definition using the before and after hooks to reset state) but it requires designing a mocking system, and maintaining consistent mocks with the required hs apis. This also means tests could become less accurate.

Thinking something similar to the jest mocking API I'm familiar with on the frontend

     ;; In a test
     (local mock mocking.spyOn(hs.window, 'focusWindowWest')
     (mocking.impl mock (fn [] nil)) ;; Create a spy that can take a mock impl
     (hs.window.focusWindowWest)
     (is.eq? (length mock.calls) 1 "Expected mock function to be called one time")

     ;; In a suite
     (after (fn [] (mocking.restore mock)))

Another option is something like clojure's with-redefs macro (or special form) that allows users to temporarily change a binding within a scope. While this would be cool and cover most of the use cases the mocking library above would, it doesn't work so well for async code.

@jaidetree
Copy link
Collaborator Author

@agzam Do you have a script to add the license\credits to each file or was that added manually?

No, I don't have a script. Btw, would you like to discuss the licensing model, etc.? I think the file headers need a revamp. Maybe we should remove Author: Ag Ibragimov and should keep the list of contributors somewhere else? After so many additions and code modifications, that info in each file doesn't stay relevant for too long.

I agree with moving author and contributors to a separate file perhaps so there's less to maintain there.

As for licensing what are your goals? Is it necessary to have the license in each file comments? Even the copyright year is already out of date.

We could setup something like https://typicode.github.io/husky/#/ with some standard pre-commit hooks that could automatically remove the header if there is one, then generate an up-to-date one on every commit. Might create some noise though once per-year when it updates though. Although personally I'm curious if there's a viable compromise that doesn't require comments in every file, but automating it would be my second choice and at least we could use the fennel CLI or babashka to update the files in a git hook script.

@agzam
Copy link
Owner

agzam commented Sep 15, 2021

I'd rather have less "bureaucracy", even if it's conveniently semi-automated. Maybe let's remove the headers altogether, wdyt?

@jaidetree
Copy link
Collaborator Author

100% on board

agzam added a commit that referenced this pull request Sep 16, 2021
#106 (comment)

Suggested-by: Jay Zawrotny <jayzawrotny@gmail.com>
jaidetree pushed a commit that referenced this pull request Sep 16, 2021
#106 (comment)

Suggested-by: Jay Zawrotny <jayzawrotny@gmail.com>
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

3 participants