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

Likely unintended behavior change in commit 41424d5 #191

Closed
doublep opened this issue Aug 12, 2020 · 7 comments
Closed

Likely unintended behavior change in commit 41424d5 #191

doublep opened this issue Aug 12, 2020 · 7 comments

Comments

@doublep
Copy link
Contributor

doublep commented Aug 12, 2020

Patterns are no longer treated as regexps because of these lines in 41424d5:

-      (buttercup--mark-skipped buttercup-suites patterns))
+      (buttercup-mark-skipped (regexp-opt patterns) t))

Function regexp-opt accepts strings not regexps.

In comparison, previously each pattern was treated as regexp:

-      (unless (cl-dolist (p patterns)
-                (when (string-match p spec-full-name)
@snogge
Copy link
Collaborator

snogge commented Aug 12, 2020

Command line patterns are documented to be substrings, not regexps in the shell script help. That is probably not prominent enough.
Was this issue on the command line or through the api?

@doublep
Copy link
Contributor Author

doublep commented Aug 12, 2020

It's not really an issue, I just remember that it used to be regexp and noticed this suspicious change. If this is intentional (i.e. if it was behaving incorrectly before), it is not a problem, I will adjust Eldev accordingly then, so that Eldev/Buttercup behaves the same as Buttercup.

@doublep
Copy link
Contributor Author

doublep commented Aug 12, 2020

Basically, I need a clarification what the intended behavior is, because I cannot pass a list of strings to buttercup-mark-skipped for it to decide how to treat them.

snogge added a commit to snogge/emacs-buttercup that referenced this issue Aug 12, 2020
This means regexes are accepted on the command line --pattern option
again.  --pattern was downgraded to substring matching in
41424d5.

Fixes jorgenschaefer#191.
snogge added a commit to snogge/emacs-buttercup that referenced this issue Aug 12, 2020
This means regexes are accepted on the command line --pattern option
again.  --pattern was downgraded to substring matching in
41424d5.

Fixes jorgenschaefer#191.
snogge added a commit to snogge/emacs-buttercup that referenced this issue Aug 12, 2020
This means regexes are accepted on the command line --pattern option
again.  --pattern was downgraded to substring matching in
41424d5.

Fixes jorgenschaefer#191.
@snogge
Copy link
Collaborator

snogge commented Aug 12, 2020

So the change was intentional but probably a bad idea.
I chose to do this change to keep the code simpler, but didn't really gain much.
Please check if #192 works for you.

@doublep
Copy link
Contributor Author

doublep commented Aug 12, 2020

Yes, seems to work fine:

~/git/buttercup$ eldev test "raised?"
Running 14 out of 162 specs.

The buttercup-failed signal
can be raised (2.14ms)

The buttercup-pending signal
can be raised (0.21ms)

The `buttercup-expect' function
with a function as a matcher argument
should not raise an error if the function returns true (0.26ms)
should raise an error if the function returns false (0.21ms)
[...]

Ran 14 out of 162 specs, 0 failed, in 95.27ms.

@snogge
Copy link
Collaborator

snogge commented Aug 12, 2020

Would you be interested in writing a section in the docs/running-tests.md file about using eldev with buttercup?
Maybe you want some stuff finished first?

@doublep
Copy link
Contributor Author

doublep commented Aug 12, 2020

Basically it would be a repeat of parts of Eldev's documentation, but if you want, I can do it.

Maybe you want some stuff finished first?

Currently missing (to bring Buttecup support in Eldev on par with Eldev) are #190 (trivial), #188 and also maybe #185 (this one should also be simple; if implemented, I will add support for this for both ERT and Buttercup in one go). Also maybe a way to reuse previous test results, but this would not be easy and would need to be designed first.

@snogge snogge closed this as completed in 6c3bb75 Aug 17, 2020
snogge added a commit that referenced this issue Aug 17, 2020
Re-enable regex patterns on the command line, fix #191
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

No branches or pull requests

2 participants