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

WIP: Add support for capturing dynamic binding environment #128

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Fuco1
Copy link
Contributor

@Fuco1 Fuco1 commented Jul 23, 2018

Fixes #127, kind of.

This is quite a bit of black magic and I'm using eval, however I can't really find any other way.

Without using :env it falls back to the old code so this should not affect any current usage.

A simple test case

(describe "env test"
  :env ((kill-ring kill-ring))

  (it "should capture environment"
    (with-temp-buffer
      (insert "asdasd")
      (goto-char (point-min))
      (kill-line))
    (expect kill-ring :to-equal (list "asdasd")))

  (it "should restore environment"
    (expect kill-ring :to-be nil)))

without the :env flag the second test fails as it is polluted by the first one doing the killing.

@@ -675,6 +675,7 @@ See also `buttercup-define-matcher'."
(cl-defstruct (buttercup-suite (:include buttercup-suite-or-spec))
;; Any children of this suite, both suites and specs
children
environment
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably unnecessary, we can track the environment in the buttercup--current-env variable (seems like a duplicate)

buttercup.el Outdated
`((let ,var
,@body))
body)))
`(buttercup-describe ,description (lambda () ,@new-body) ',env)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing ',env here is really ugly... any ideas?

(mapc
(lambda (binding)
(if (consp binding)
(setf (symbol-value (car binding)) (eval (cadr binding)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

eval :( But, since these are functions and we are passing data I don't see any good way, other than walking the trees in describe macro and looking for it forms and installing let-bindings there.

(funcall body-function))
(mapc
(lambda (binding)
(setf (symbol-value (car binding)) (cdr binding)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

setf might be unnecessary, maybe set would work the same? (since we are setting the symbol value)

@jorgenschaefer
Copy link
Owner

Thank you for the PR. Wouldn't it be easier and more readable to just setq the variable in before-each, or maybe just support an around-each type of thing?

@Fuco1
Copy link
Contributor Author

Fuco1 commented Jul 27, 2018

@jorgenschaefer Problem with setq is that it corrupts the global state. It would not be a problem if buttercup run each test case in a separate Emacs instance but that would be crazy slow.

It might work for one describe set if I always initialize it but basically it would force me to do it absolutely everywhere in the entire test suite. For example, imagine I set case-fold-search to nil in one suite, then for the rest of the execution it is going to have this value.

What can be done with the around-each would be to get the value and then re-set it which would work but we must take care of errors also (i.e. never forget to re-set the value). The user would have to do this manually for every variable then. This is basically what this PR does.

Theoretically, this might work

(describe "Backup"
  :var (kill-ring-backup)

  (before-each
    (setq kill-ring-backup kill-ring)
    (setq kill-ring ("foobar")))

  (after-each
    (setq kill-ring kill-ring-backup))

  (it "kill ring should not be empty"
    (expect (< 0 (length kill-ring)) :to-be-truthy)))

Then the question is what are the guarantees on after-each.

@Fuco1
Copy link
Contributor Author

Fuco1 commented Jul 27, 2018

What you described is a common idiom in javascript but remember that there the scoping works different

describe('getProductValue', () => {

    let edsnDown

    describe('when edsn is up', () => {

        beforeEach(() => {
            edsnDown = false
        })

        it("should return '1' when we have elk only", () => {
            expect(getProductValue(true, edsnDown)).toEqual('1')
        })

        it("should return '2' when we have elk and gas", () => {
            expect(getProductValue(false, edsnDown)).toEqual('2')
        })

    })

})

here the edsnDown will never spill outside of the describe... in Emacs special variables will :(

@jorgenschaefer
Copy link
Owner

How about around?

(describe "Backup"
  (around-each block
    (let ((kill-ring '("foobar")))
      (funcall block)))

  (it "kill ring should not be empty"
    (expect (< 0 (length kill-ring)) :to-be-truthy)))

@Fuco1
Copy link
Contributor Author

Fuco1 commented Aug 3, 2018

@jorgenschaefer That is actually really cool idea. You were doing some ruby lately heh? :D

Yea I think that could work if implemented properly. I'll try to take a look on that if you don't mind (or have it already implemented)

@jorgenschaefer
Copy link
Owner

Am I getting a Ruby dialect? Help! :-D I have nothing implemented and I would love such a feature, thank you!

@bbatsov
Copy link

bbatsov commented Sep 16, 2018

@Fuco1 Great work so far with this! Looking forward to its completion!

P.S. As a Rubyist myself I welcome any suggestion to get buttercup closer to what RSpec offers - it's a pretty amazing framework.

@wyuenho
Copy link

wyuenho commented Aug 10, 2022

Is there any progress on this? This is quite an important feature missing in buttercup. Without it, "mocking" special variables becomes quite annoying.

@Fuco1
Copy link
Contributor Author

Fuco1 commented Aug 10, 2022

I probably won't work on this in any reasonably close future.

@snogge
Copy link
Collaborator

snogge commented Aug 10, 2022

This PR is probably older than my maintainership, and as it was marked WIP I never gave it much thought.
I don't think I'll get to it soon considering how little time I have to spend on this project.
If you have any specific problem, please file a separate issue. Maybe we can figure something out.

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.

:var does not seem to work with dynamic vars
5 participants