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: e2eテストが動かないのを修正 #1276

Merged
merged 75 commits into from
Apr 11, 2023

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Apr 3, 2023

内容

e2eテストの色々を修正します。

  • playwrightにwatchオプションはなかったのでPWTEST_WATCH=1で対応。(参照
  • electronをそれぞれのテストで起動するように。
  • Workflowにe2eテストを追加

関連 Issue

スクリーンショット・動画など

(なし)

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner April 3, 2023 02:26
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team April 3, 2023 02:26
@Hiroshiba
Copy link
Member

PRありがとうございます!

最近playwrightにwatchモードができたっぽいのですが、こちらはどうでしょう 👀

@sevenc-nanashi
Copy link
Member Author

UI表示の強制っぽかったのでパスしてましたが、PWTEST_WATCH=1 というものがあるんですね。そっちを使うようにしました。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

いくつかコメントがありますが、なぜplaywright testだけでなくvite serveを経由する必要が出てきているのか知りたいです。

提案1:
せっかく有用なnpm runタスクが増えても、どれが何をするのかわからないかもしれません。
READMEで案内するのはどうでしょう。

提案2:
今の仕組みだと、e2eテストがいつの間にか壊れても気づけないかもです。
Github Actionsのテストに加えるのはどうでしょう。

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@sevenc-nanashi
Copy link
Member Author

いくつかコメントがありますが、なぜplaywright testだけでなくvite serveを経由する必要が出てきているのか知りたいです。

元々はPlaywrightのbeforeAllフックでViteのサーバーを立ち上げていました。が、watchすると変更毎にviteのサーバーが立ち上げ直されるので、別のscriptに分離しました。
その後(現在)、playwrightのコンフィグに開発サーバーを立ち上げられるオプションがあるのを知ったので書き換えました。(参照

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Apr 5, 2023

Actions、何故か動きませんね。
デバッグログ有効化してみましたが変なログもなく。

@sevenc-nanashi sevenc-nanashi marked this pull request as draft April 5, 2023 12:58
@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 6, 2023

起動に30秒かかってるとか・・・?
electron側のログとか参考になるかもと思いました。(出てるのかわかりませんが)

@sevenc-nanashi
Copy link
Member Author

×Waiting for background.js to be built...
background.js is built.
  pw:browser <launching> D:\a\voicevox\voicevox\node_modules\electron\dist\electron.exe -r D:\a\voicevox\voicevox\node_modules\playwright-core\lib\server\electron\loader.js --inspect=0 --remote-debugging-port=0 . +0ms
  pw:browser <launched> pid=4396 +10ms
  pw:browser [pid=4396][out]  +19ms
  pw:browser [pid=4396][err] Debugger listening on ws://127.0.0.1:63049/9bcc83a2-67a5-4ad6-9048-0f62eab003f9 +21ms
  pw:browser [pid=4396][err] For help, see: https://nodejs.org/en/docs/inspector +0ms
  pw:browser <ws connecting> ws://127.0.0.1:63049/9bcc83a2-67a5-4ad6-9048-0f62eab003f9 +1ms
  pw:browser <ws connected> ws://127.0.0.1:63049/9bcc83a2-67a5-4ad6-9048-0f62eab003f9 +10ms
  pw:browser [pid=4396][err] Debugger attached. +1ms
  pw:browser [pid=4396][out] Environment: test, appData: voicevox-test +278ms
  pw:browser [pid=4396] <kill> +30s
  pw:browser [pid=4396] <will force kill> +0ms
  pw:browser [pid=4396] taskkill stdout: SUCCESS: The process with PID 4396 (child process of PID 3836) has been terminated.
  pw:browser  +75ms

@Hiroshiba
Copy link
Member

pw:browser [pid=4396] +30s

30秒でkillされてそう・・・?

@sevenc-nanashi
Copy link
Member Author

うーん、60秒でも起動しませんね。
となるとPlaywrightとかそっちのバグ説が濃厚…?

version:
description: "VOICEVOX ENGINEのバージョン。"
value: ${{ steps.result.outputs.version }}
runs:
Copy link
Member

Choose a reason for hiding this comment

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

結構長いですね・・・。

このaction.yml全部をshellスクリプトを書いてbuildディレクトリあたりに入れておけば、手元でデバッグしやすいかもです。
あとREADMEでエンジンダウンロードも案内できて嬉しそう。まあご興味あれば・・・!

Copy link
Member Author

@sevenc-nanashi sevenc-nanashi Apr 9, 2023

Choose a reason for hiding this comment

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

Actionsに依存してる部分がそこそこあって大変そうなので別の機会にやろうと思います

@Hiroshiba
Copy link
Member

修正ありがとうございます、もうちょいだと思います!!

ちょっとテスト見てたのですが、test: e2eに失敗していそうなのにActionsが止まらず成功していそうでした。
Fix: 変数展開を修正 e0c59a3のときのテストです。
image

ちなみに最新のはpassedになっていました。
image

これ原因とかわかりそうでしょうか。(exitコードが1ではないとか・・・?)

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Apr 10, 2023

1回目で失敗し、2回目で成功した時の表示だと思います。
Playwrightがエンジンをkillできてなく、2回目はエンジン待機がなかったから成功した…っていう予想です。
エンジンを別で起動した方が安定するかもしれない。

参照:https://blog.cybozu.io/entry/2020/12/23/100000

Flaky Testとは、実行結果が不安定なテストのことです。 コードを変更していないにもかかわらず、実行するたびにテストが成功したり失敗したり結果が変化するため、原因が追及しにくく非常にやっかいな問題です。

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 10, 2023

あー、なるほどです! flakyってそういうことなんですね。

エンジンを別起動にすると安定しそうですが、エンジン起動シーケンスを通らなくなってしまうのは残念かもです。
electron.launchtimeout: process.env.CI ? 0 : 30000,のとこで起動タイムアウトをもっと伸ばしておくと安定するかも?

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Apr 10, 2023

エンジン起動シーケンスを通らなくなってしまうのは残念かもです。

ダミーの、何もせず残る実行ファイルを用意しても良さそうに感じました。起動検知のために、Playwright側でVV_CHECK_NONCEみたいな環境変数を適当な値にセットし、その名前でファイルを作成するみたいな実行ファイルを用意しても良さそう?ファイルの存在チェックで起動確認できるはずです。

await Deno.writeTextFile(Deno.env.VV_CHECK_NONCE, "")
await new Promise(() => undefined)

みたいな。

@Hiroshiba
Copy link
Member

なるほどです、そういう手もありますね。
整理してみたのですが、テストしたい範囲がカバーできてるかどうかが考える上で重要だなと思いました。

例えば「起動したら「利用規約に関するお知らせ」が表示される」テストの場合、初回起動時に起動シーケンス含めてちゃんと動作してほしいので、通常のエンジンで実際に起動するのが良さそうに思いました。

でも例えば「テキスト欄の追加や消去が一通りできる」みたいなテストの場合、エンジンは使い回したりそもそもモックでも良いなと思いました。

こう考えると、今回はとりあえず「エンジンが本当に起動する」のが良さそうに思いました!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

とはいえ今回のPRで動くようになったので、いったんマージに進むのが良いのかなとちょっと思いました。

tests/e2e/example.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!

修正ありがとうございました!!
テストどんどん足していってみたいですね!!

@Hiroshiba
Copy link
Member

問題ないと思うのでマージします!

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.

2 participants