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

Fix: eask options should not be passed to buttercup #281

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

joshbax189
Copy link
Contributor

E.g. before

> eask test buttercup --no-color
Package-file seems to be missing `buttercup.el'
  - Skipping buttercup (20240904.2311)... already installed ✗
Opening directory: No such file or directory, /home/naver/git/eask-cli/test/commands/test/buttercup/--eask--no-color

Fixed by setting command-line-args-left to the result of eask-args before calling buttercup-run-discover.

Another weird bug I noticed was that directories that are not children of the current directory are treated like empty directories.
Buttercup always fails, reporting 'No suites defined', even if there are test suites there. This can be kind of confusing, so I added an error for if the user tries to do this using ../.
E.g.

> eask test buttercup ../something
Buttercup cannot run in parent directories

However, this is also a problem when using an absolute directory that is not a child too

> pwd
/path/to/something/foo # something is the parent of the current dir
> eask test buttercup /path/to/something
No suites defined

You get this output even if there are buttercup tests in /path/to/something

If you want, I can test absolute paths too (i.e. check that every argument is on the load path/is reachable from the current directory), but I thought it might be a bit obscure? What do you think, is it worth checking?

@jcs090218
Copy link
Member

Fixed by setting command-line-args-left to the result of eask-args before calling buttercup-run-discover.

👍

Another weird bug I noticed was that directories that are not children of the current directory are treated like empty directories.
Buttercup always fails, reporting 'No suites defined', even if there are test suites there. This can be kind of confusing, so I added an error for if the user tries to do this using ../.

You are right. It's weird that ../ doesn't work. 🤔 Although this is confusing, I think it should be resolved from the upstream rather than by Eask.

If you want, I can test absolute paths too (i.e. check that every argument is on the load path/is reachable from the current directory), but I thought it might be a bit obscure? What do you think, is it worth checking?

I don't think it's needed since those bugs are probably from buttercup itself and not from Eask.

For example, I have been testing Elsa in the past, but eventually, I had to get rid of it (see https://github.com/emacs-eask/cli/blob/master/test/commands/local/run.sh#L103) due to its instability. 🤔

@joshbax189
Copy link
Contributor Author

I think it should be resolved from the upstream rather than by Eask.

So keep or remove the existing warning? Maybe keeping it would prevent github issues from nosy people like me ;)

@jcs090218
Copy link
Member

So keep or remove the existing warning? Maybe keeping it would prevent github issues from nosy people like me ;)

I prefer to remove it since making some noise can cause more people's attention. 🤔 But I guess reporting an error has the same effect.

Let’s keep it and add a comment linking to this PR, so I can easily find and remember the reason behind this code. 😉

@jcs090218 jcs090218 added the bug Something isn't working label Nov 17, 2024
@joshbax189
Copy link
Contributor Author

Done! Turns out it was pretty easy to check for both types of paths anyway. Also changed the warning message to include the specific path, just in case multiple paths were given.

> ~/git/eask-cli/bin/eask test buttercup ../ert
Buttercup cannot run in parent directory: ../ert

@jcs090218 jcs090218 merged commit 1da0201 into emacs-eask:master Nov 18, 2024
142 of 143 checks passed
@jcs090218
Copy link
Member

Thank you! Merged! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants