Port test from popup-interactive-test.el(20/20)

uk-ar commented 11 years ago

This pull request include the test below.

popup-tip with height
popup-tip with scroll-bar
popup-tip with min-height
popup-menu*
popup-cascade-menu
popup-next
popup-previous
popup-select
popup-scroll-down
popup-scroll-up

uk-ar commented 11 years ago

Now, rest of popup-interactive-test.el's test is ported. @m2ym, @tkf could you review this?

m2ym commented 11 years ago

Looks great to me. Two suggestions here:

Matching the buffer contents would make tests more legible and robust in some cases.
popup-test-helper-points-to-columns should use (popup-current-physical-column) or (car (posn-col-row (posn-at-point))) instead of (current-column).

uk-ar commented 11 years ago

日本語ですみません。 2. のposn-col-rowを使う際に問題が発生しました。 batch-modeではposn-at-point(というかpos-visible-in-window-p)がうまく動かないようです。具体的には

(with-temp-buffer
  (switch-to-buffer (current-buffer))
  (delete-other-windows)
  (should (posn-col-row (point) (selected-window)))
  )

ではposn-col-rowがnilを返してしまいます。(window-systemや--no-window-systemでは期待通りの結果を返します) 表示系の問題かと思い、make-frame-visible,redisplay,recenter,noninteractiveなどいろいろ試しましたが、どれもうまく行きません。temp-bufferではなくget-buffer-createなどしても同様でちょっとお手上げかもしれません。

uk-ar commented 11 years ago

1.に関してはpopup-listなどを使わず、popup-test-helper-match-pointsによる文字列マッチングをするという認識で合ってますか？こちらの方が記述量は増えますが、仕様変更には強くなると思っています。(:min-heightに関してはどうしてもtext-propertyのscanが入ってしまいそうですが...)

m2ym commented 11 years ago

(1) というより、オーバーレイを考慮した buffer-string のような関数を定義して、次のようにテストする、という話です。

(should (equal (buffer-contents)
"item1
"item2item21
      item22"))

(2) バッチモードをあきらめるという手があります。似たような理由によりpopwin.elもバッチモードでのテストをあきらめています。Travis CIが使えなくなるため、現実的には、インタラクティブモードの時は posn-col-row を使い、バッチモードのときは current-column でごまかす、という具合になるかと思います。 popup-current-physical-column が参考になるかと思います。

uk-ar commented 11 years ago

(2)了解です。popup-current-physical-columnを参考に、バッチモードとインタラクティブモードで処理を切り替えます。インタラクティブモードでのテストを忘れないようにしないといけませんね。今のところ折り返しを意図したテストはないので、期待値も変わらない気がします。 (1)はまだ理解が追いつかないところがあります。例から、ヒアドキュメントで複数行を渡すように見えますが、判定の仕方が2つぐらいありそうです。

オーバーレイ部分を集めた文字列を作りそれとの一致するかを確かめる。
元のバッファの内容+オーバーレイの内容に対して矩形比較する。 1.の方が簡単ですが開始位置のずれや折り返しが検出出来なくなります。2. はそれなりに大変かもしれません。どちらにしましょうか？

m2ym commented 11 years ago

の方が、テストが作りやすく、しかも読みやすいと思います。 EmacsWiki の OverlaysToText が参考になると思います。

m2ym commented 11 years ago

試作してみました。 popup が挿入する改行をどこかで取り除く必要があります。

(defun buffer-contents ()
  (with-output-to-string
    (loop with start = (point-min)
          for overlay in (sort* (overlays-in (point-min) (point-max))
                                '< :key 'overlay-start)
          for overlay-start = (overlay-start overlay)
          for overlay-end = (overlay-end overlay)
          for prefix = (buffer-substring-no-properties start overlay-start)
          for befstr = (overlay-get overlay 'before-string)
          for substr = (or (overlay-get overlay 'display)
                           (buffer-substring-no-properties overlay-start overlay-end))
          for aftstr = (overlay-get overlay 'after-string)
          do (princ prefix)
          unless (overlay-get overlay 'invisible) do
          (when befstr (princ befstr))
          (princ substr)
          (when aftstr (princ aftstr))
          do (setq start overlay-end)
          finally (princ (buffer-substring-no-properties start (point-max))))))

uk-ar commented 11 years ago

遅くなってしまい、申し訳ありません。矩形っぽく一致するかを比較する関数を試作しました。

(defun popup-test-helper-line-move-visual (arg)
  "This function is workaround. Because `line-move-visual' can not work well in
batch mode."
  (let ((cur-col
         (- (current-column)
            (save-excursion (vertical-motion 0) (current-column)))))
    (vertical-motion arg)
    (move-to-column (+ (current-column) cur-col))))

(defun popup-test-helper-rectangle-match (str)
  (goto-char (point-max))
  (let ((strings (split-string str)))
    (search-backward (car strings))
    (every
     'identity
     (mapcar
      (lambda (elem)
        (popup-test-helper-line-move-visual 1)
        (looking-at (regexp-quote elem)))
      (cdr strings)))))

このpopup-test-helper-rectangle-matchを使用すると

OK
NG(途中で折り返しが入る) https://github.com/auto-complete/popup-el/issues/21 https://github.com/auto-complete/auto-complete/issues/199
NG(ずれてる)

という感じで判定できます。実際の判定部分は

(ert-deftest popup-test-no-truncated ()
  (popup-test-with-temp-buffer
   (progn
     (insert (make-string (- (window-width) 4) ? )) (insert "Foo\n")
     (insert (make-string (- (window-width) 4) ? )) (insert "Bar\n")
     (insert (make-string (- (window-width) 4) ? )) (insert "Baz\n"))
   (should (eq t (popup-test-helper-rectangle-match "\
Foo
Bar
Baz")))
   ))

(ert-deftest popup-test-truncated ()
  (popup-test-with-temp-buffer
   (progn
     (insert (make-string (- (window-width) 2) ? )) (insert "Foo\n")
     (insert (make-string (- (window-width) 2) ? )) (insert "Bar\n")
     (insert (make-string (- (window-width) 2) ? )) (insert "Baz\n"))
   (should (eq nil (popup-test-helper-rectangle-match "\
Foo
Bar
Baz")))
   ))

(ert-deftest popup-test-misaligned ()
  (popup-test-with-temp-buffer
   (progn
     (insert (make-string (- (window-width) 6) ? )) (insert "Foo\n")
     (insert (make-string (- (window-width) 5) ? )) (insert "Bar\n")
     (insert (make-string (- (window-width) 4) ? )) (insert "Baz\n"))
   (should (eq nil (popup-test-helper-rectangle-match "\
Foo
Bar
Baz")))
   ))

です。これでよさそうなら、上記buffer-contentsと組み合わせて置き換えようとおもいます。どうでしょう？

uk-ar commented 11 years ago

1d692e4 でいくつかのテストをbuffer-contents(popup-test-helper-buffer-contents)とpopup-test-helper-rectangle-matchで置き換えてみました。テストのセットアップ部分（popupを作るまで）と確認部分(should)の切れ目がはっきりするのでかなり見通しがよくなる印象です。問題がなさそうならば、このまま置き換えを進めようと思います。

m2ym commented 11 years ago

そうか、折り返しの問題があるのか、全然考えてなかった。buffer-contentsじゃなくて、折り返し等も含めてウィンドウをダンプするdump-windowみたいなのを作ると良いのかもしれない。

m2ym commented 11 years ago

‘popup-test-helper-rectangle-match‘がかなりトリッキーな印象ですが、他は特に問題ないと思います。

tkf commented 11 years ago

この PR 全然おえてませんが、最新の ecukes ではデフォルトで emacs -nw 内でテストするようになったようです(script モードもありますが)。 ecukes を使えばインタラクティブモードのみのテストで済むかもしれません。

GUI モードでも起動できたり、デバッガも起動できるようになってるようで、最初のころのディスカッションで私が反対していた理由がほとんど潰されてました。。。

http://ecukes.info/

m2ym commented 11 years ago

公式サイトがEmacs関連とは思えないぐらいオシャレですね

uk-ar commented 11 years ago

@m2ym そうですね。popup-test-helper-rectangle-matchの処理は確かに微妙です(せめてline-move-visualが使えれば…) しかし、折り返し含めてdumpをしても、左端以外のカラムでpopupする場合は垂直移動が欲しいです。 @tkf さん、情報ありがとうございます。batch-modeの挙動には悩まされているので、emacs -nwでできるとうれしいです。とりあえずコードの整理をして、移行を検討するのもありかと思います。

uk-ar commented 11 years ago

試しでecukesでいくつかテストを書いてみました。 https://github.com/uk-ar/popup-el/commit/de56f4532b66d1f228febcb2af487315570fbf93 上から３つ

features/popup-el.feature
features/step-definitions/popup-el-steps.el
features/support/env.el

がecukesで書いたテスト。最後の

tests/popup-test2.el

がertで書いた同じ内容のテストです。 ecukesで書くと、テストケースは読みやすくなるんですが、トータルの行数は増えそうです（1.5倍ぐらい？）あとは、なぜか実行するときのテストの挙動に微妙にくせがあるので、別のバッドノウハウがたまりそうです...

ちょっと感想が欲しいのですが、いかがでしょうか？

tkf commented 11 years ago

https://github.com/uk-ar/popup-el/commit/de56f4532b66d1f228febcb2af487315570fbf93 にちょっとコメントしてみました。

"テストの挙動に微妙にくせがある" とは具体的にはどんな癖ですか?

ecukes を別プロジェクトちょっと使ってみたんですが、起動した端末の大きさにテスト結果が左右されるのでそこは注意が必要かもしれません。 Window の大きさに依存するテストが出てきたら、 tmux で幅と高さを指定して起動するみたいなことをする必要があると思います。 ecukes 自体に入れて欲しい機能ですが。

あと、 popup-test-helper-rectangle-match ですが、比較対象の str を二つ引数にとるか内部で popup-test-helper-buffer-contents を呼ぶとすっきりするのでは、と思いました。

uk-ar commented 11 years ago

コメントありがとうございます。コメントにはインラインで返信しました。

"テストの挙動に微妙にくせがある" とはM-x ecukes で動いていたline-move-visualが、コマンドラインからだとうまく動かず、https://github.com/uk-ar/popup-el/blob/de56f4532b66d1f228febcb2af487315570fbf93/features/step-definitions/popup-el-steps.el#L61-L63 みたいな処理を入れる必要があって微妙な気分になったぐらいです。

Window の大きさに依存するテストは考えてませんでした。最低限欲しい横幅があったので、https://github.com/uk-ar/popup-el/blob/357f037615c8bf4aa885493a6b6b1041e0a5dfd1/tests/popup-test.el#L7-L8 みたいなことをしてましたが、考えないといけませんね...

popup-test-helper-rectangle-matchの件はpopup-test-helper-popup-exist-p( https://github.com/uk-ar/popup-el/blob/de56f4532b66d1f228febcb2af487315570fbf93/tests/popup-test2.el#L75-L80 )にpopup-test-helper-rectangle-matchを統合する認識であってますか？確かにpopup-test-helper-rectangle-match単体では使わなさそうなので問題はなさそうですが、ちょっと役割が多い気もします（あまりこだわりはないです）

tkf commented 11 years ago

popup-test-helper-rectangle-match の件は、こんな感じのほうが見通しが良くなるのでは、という意味でした。

(defun popup-test-helper-rectangle-match (real desired)  ; 第一引数を追加
  (with-temp-buffer
    (insert real)
    (goto-char (point-max))
    (let ((strings (split-string desired "\n")))
      (search-backward (car strings) nil t)
      (every
       'identity
       (mapcar
        (lambda (elem)
          (let ((temporary-goal-column (or temporary-goal-column
                                           (current-column))))
            (line-move-visual 1 t))
          (looking-at (regexp-quote elem)))
        (cdr strings))))))

この temporary-goal-column の hack は確かに微妙ですね... 普通にコマンドラインから ecukes を立ち上げると M-x ecukes を emacs -nw の中でやったものと同等だと思ったんですが。

uk-ar commented 11 years ago

popup-test-helper-rectangle-match の第一引数を追加するとすれば

popup の始まり行数をチェックする関数
popup の終わり行数をチェックする関数
popup の始まりカラム数をチェックする関数
popup 内に特定文字列が含まれていないことをチェックする関数(min-heightやスクロールで必要)

あたりも第一引数の追加が必要そうですね。いっそのこと第二案の popup-test-helper-rectangle-match および上記チェック関数内部で popup-test-helper-buffer-contents を呼び出すのもアリな気がしますね。 insert を外に出したかった理由は（いくつあるかわからなかったので）なるべく個々のチェック関数を書かずに済ませたかったからぐらいです。

uk-ar commented 11 years ago

あと１点 ecukes を使う時に困りそうなのが、popupの始まり行数が最終行の一つ上とかを確かめたい時です。ert だと (- (window-body-height) 1) とか書いていたのですが、どう書くのがよいんでしょうか？（一応サンプルに目を通したつもりですが、それらしいStepが見つかりませんでした）今のところ、文字列で受け取って eval するのかと考えてますが、そんなのでいいんでしょうか？

tkf commented 11 years ago

こんな感じの step を書くのが自然だと思います

When I create these popup items at window bottom:
    | Item   |
    | item 1 |
    | item 2 |

パラメタが欲しければ、こんな感じでしょうか

When I create these popup items at 2 lines before window bottom:
    | Item   |
    | item 1 |
    | item 2 |

チェック用関数が、 popup が作られたバッファではなく「比較用の文字列が入ったバッファ」に居ることを前提としている点が、素直じゃないなと感じた理由です。それで、 popup が作られたバッファに居ることを前提にする(popup-test-helper-buffer-contents を呼ぶ)か、どのバッファにいても動くようにする(文字列を2つ渡す)かしたほうが良いのでは、と思いました。

tkf commented 11 years ago

"When I popup these items ..." のほうが短くて良いですね

uk-ar commented 11 years ago

なるほど、提示されたstepでできそうですね。ありがとうございます。

チェック用関数が、 popup が作られたバッファではなく「比較用の文字列が入ったバッファ」に居ることを前提としている点が、素直じゃないなと感じた理由です。

納得しました。とすると、チェック用関数はバッファ内容をダンプしてることすら意識させない(内部で popup-test-helper-buffer-contents を呼び出す)のがいいと感じました。とりあえず内容を反映しながら続きを書いてみます。

uk-ar commented 11 years ago

とりあえず、議論の内容を一通り反映させてみました。 05f5492 はpopupの開始行数、終了行数を判断するのにfaceの情報が必要だったので popup-test-helper-buffer-contents に手を入れました。 ecukes 版も書いてみようと思いますが、別のPRにしようと思ってます。私事で申し訳ないですが、子供が生まれたのでまたしばらく活動が停滞するかもです。

uk-ar commented 11 years ago

現時点で通っていないテストも追加しました。こんな感じでpendingというキーワードとともにコメントアウトしているので、一通り成功します

m2ym commented 11 years ago

マージしてしまってもかまいませんか？

uk-ar commented 11 years ago

一応、区切りはついています。ぱっと見て良さそうならマージをお願いします。残作業として

[ ] ecukes版
[ ] autocompleteのテストリストの一部

がありますが、別PRで進めようと思います。

m2ym commented 11 years ago

了解です。どうもありがとうございます。

auto-complete / popup-el

Port test from popup-interactive-test.el(20/20) #23