Skip to content

Commit

Permalink
ivy-test.el (counsel-find-file-with-dollars): Add test
Browse files Browse the repository at this point in the history 
Re #2012
  • Loading branch information
@abo-abo
abo-abo committed Apr 10, 2019
1 parent 115efb7 commit 7e73580
Showing 1 changed file with 18 additions and 4 deletions.
22 changes: 18 additions & 4 deletions ivy-test.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,17 @@

(global-set-key (kbd "C-c e") 'ivy-eval)

(defun ivy-with (expr keys)
(cl-defun ivy-with (expr keys &key dir)
"Evaluate EXPR followed by KEYS."
(let ((ivy-expr expr)
(inhibit-message t))
(execute-kbd-macro
(vconcat (kbd "C-c e")
(kbd keys)))
(save-window-excursion
;; `execute-kbd-macro' doesn't pick up `default-directory'
(when dir
(dired (expand-file-name dir (counsel-locate-git-root))))
(execute-kbd-macro
(vconcat (kbd "C-c e")
(kbd keys))))
ivy-result))

(defun command-execute-setting-this-command (cmd &rest args)
Expand Down Expand Up @@ -1071,6 +1075,16 @@ a buffer visiting a file."
"C-p C-m")
""))))

(unless (file-exists-p "test")
(shell-command "git clone -b test --single-branch https://github.com/abo-abo/swiper/ test"))

This comment has been minimized.

Sign in to view

Copy link
@basil-conto

basil-conto May 2, 2019

Collaborator

Please do not unconditionally force this upon anyone who tries to run the test suite.

This comment has been minimized.

Sign in to view

Copy link
@mookid

mookid May 2, 2019

Contributor

Actually you can clone a local repository, it would not use the network.

This comment has been minimized.

Sign in to view

Copy link
@basil-conto

basil-conto May 2, 2019 •
edited
Loading

Collaborator

Whether it uses the network is only part of the problem. This form should not be run every time the test suite is run. There shouldn't be tests in the master branch that depend on data from other branches.

This comment has been minimized.

Sign in to view

Copy link
@abo-abo

abo-abo May 6, 2019

Author Owner

My motivation for this was to avoid having a large file list in the master branch.
I don't expect the files in the test branch to change often if at all; it's a waste to bloat the file list with them.

This comment has been minimized.

Sign in to view

Copy link
@basil-conto

basil-conto May 6, 2019

Collaborator

it's a waste to bloat the file list with them.

Oh, I didn't realise these were empty files being used solely for their names. There is a simple and canonical solution to this problem: instead of needlessly littering the directory tree with empty files, create temporary files on the fly via make-temp-file, and delete them in an unwind-protect.

This comment has been minimized.

Sign in to view

Copy link
@abo-abo

abo-abo May 6, 2019

Author Owner

We could do that. But having the whole directory readily available for manual interaction / debugging is also a plus.

Additionally, further tests might be added with non-empty files.

I'm thinking of two additional ways of handling it:

  • Keep everything in master; use .ignore and counsel-rg for selecting the file list.
  • Have the master git branch be the dependency of test instead of the other way around.

The current approach also helps us avoid including large files into the master branch. Suppose in the future we add a performance test to swiper with a 1Mb file. Then it's better to not pollute the master branch with it.

This comment has been minimized.

Sign in to view

Copy link
@basil-conto

basil-conto May 6, 2019

Collaborator

But having the whole directory readily available for manual interaction / debugging is also a plus.

That's still possible, even if the directory is created on the fly. First, ERT tests support interactive debugging. Second, if you replace the unwind-protect I mentioned earlier with (condition-case err ... (ert-test-failed ...)), then the test directory can be left around for further debugging instead of being unconditionally deleted.

Additionally, further tests might be added with non-empty files.

Again, something that can either be created on the fly, or live permanently on the same branch as the tests.

Keep everything in master; use .ignore and counsel-rg for selecting the file list.

As long as this is customisable, sure.

Have the master git branch be the dependency of test instead of the other way around.

I think it's neither necessary, clean, nor a good idea for branches to depend on each other like this. Git branches are meant for self-contained and independent project state. Otherwise, they can go out of sync, and then you have two (or more) problems, not just one. What if a new development branch is created to test some new feature? Then it might also need its own test branch.

When one checks out a branch, its docs, code, and tests should all be consistent with one another, and no further checkouts should be required, especially not checking out one branch of the same repository as a subdirectory of another branch, just for the sake of a smaller file list. This is an abuse of Git branches. You could achieve a similar effect with Git submodules, but that's still bad organisation, as submodules can be pushed independently of their parent.

The current approach also helps us avoid including large files into the master branch. Suppose in the future we add a performance test to swiper with a 1Mb file. Then it's better to not pollute the master branch with it.

Why does the file's size matter? The 1MB file will be stored in your repository in one way or another. I don't understand your reservations against "polluting" the file list when the current approach of checking out a separate branch as a subdirectory also pollutes the file list.

In conclusion, test files should either live permanently in the same branch as the code testing them, or should be created by said tests on the fly. If you don't want to see data files during file name completion, just add their directory to the appropriate ignore list.

This comment has been minimized.

Sign in to view

Copy link
@abo-abo

abo-abo May 7, 2019

Author Owner

Why does the file's size matter? The 1MB file will be stored in your repository in one way or another

We still can have git clone -b master for a fast clone.

But you're probably right, we should move to either on-the-fly files or files in master for tests.

(ert-deftest counsel-find-file-with-dollars ()
(should (string=
(file-relative-name
(ivy-with '(counsel-find-file) "fo C-m"
:dir "test/find-file/files-with-dollar/"))
"test/find-file/files-with-dollar/foo$")))

(provide 'ivy-test)

;;; ivy-test.el ends here

0 comments on commit 7e73580

Please sign in to comment.