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

[DNM] コマンドの自動テストでストリームしながらキャプチャするためにteeコマンドを使う #458

Closed
wants to merge 2 commits into from

Conversation

omochi
Copy link
Contributor

@omochi omochi commented May 19, 2024

このPRのステータス

#457 に依存しているので、それがマージされるまでは Draft にしておきます。

レビューの用意ができました。CIを確認中です。

実装を修正中です

課題

テストの swiftRun は、プロセスの終了を待ってから、
パイプの中身を取り出しています。
これには課題があります。

パイプが詰まるかもしれない

テストでは swift run carton dev などのコマンドを実行していますが、
これはマシンの状況によっては、
Swift ツールチェーンのダウンロードから、
テスト対象プロジェクトのビルドなどの多くの処理をします。
その間パイプが全くドレインされないので、
OSにバッファされ続けます。

これが最大容量に達すると、
書き込み側プロセスがフリーズするとか、
シグナルによってプロセスがキルされる恐れがあります。

ちなみにLinuxでは容量は 64KB ぐらいらしいので、
which とか env のような小さなコマンドでは問題ない実装ですが、
たくさん出力が生じるようなインタラクティブなコマンドでは不適切です。

処理が終わるまで結果が見えない

ツールチェーンのダウンロードなどが生じる場合、
一つのプロセス実行に3分ほどかかる場合があります。
この間全く出力がコンソールに現れないため、
状況がわからないし、
なんらかの理由でプロセスがフリーズしている場合、
ゆっくり進んでいる場合と区別がつかず開発しづらいです。

背景

テストコードでは、ほとんどの場合がステータスコードをチェックしているだけですが、
一件だけプロセスの出力内容に対するテストが書かれています。

このようなプロセス出力に対するテストは有効で、
むしろもっと増やしていくべきでしょう。

提案

課題の解決にはプロセス出力を溜め込まずにストリームするのが簡単ですが、
上述の通りテストでは結果をキャプチャしたいニーズもあります。

そこで tee コマンドを使用して、
コンソールにストリームしつつ、ファイルにも溜め込みます。
プロセスが終了したらファイルから読み込んで、
内容の検証のニーズに備えます。

実装

メインのプロセスとteeプロセスを繋げて使う場合は、
まずそれぞれのプロセスの停止を待ってから、
その後で出力バッファファイルを読み込みます。

プロセスの停止を待つ処理ですが、Foundation.waitUntilExit はブロッキング処理なのが問題です。
swift concurrencyと干渉する恐れがあるので、
asyncのインターフェースを提供すべきでしょう。

その後、プロセスの結果を受け取り、ステータスのチェックをします。
実装の簡単さとデバッグしやすさのため、
そのチェックメソッドは失敗すれば例外を投げ、
例外は出力内容をダンプできると良いでしょう。

teeプロセスの書き込み先については、
テンポラリファイルを適切に準備する必要があります。
これは carton モジュールに実装があるので、
これを CartonHelpers モジュールに移動してきて、
テストから共用します。

以上の要求に応えるため、
FoundationProcessResult 型を実装します。
また、これを握って飛んでいく FoundationProcessResult.Error 型を実装します。

これはほぼ CartonHelpers.ProcessResult と同じものですが、
CartonHelpers.Process ではなく、
Foundation.Process と合わせて使うように設計されています。

ちなみにですが、CartonHelpers.ProcessError という型が Process+run.swift で実装されていますが、
実は CartonHelpers.ProcessResult.Error という型があって、ほぼ重複しています。

将来の目標

現在 carton において、 Foudation.ProcessCartonHelpers.Process という、
2つのほぼ同じ役割の型が存在し、利用するコードが混在するのでコードが紛らわしいです。

今回実装した FoundationProcessResult とその関連メソッドがあれば、
CartonHelpers.Process でなければできない事はほぼなくなります。
将来的には CartonHelpers.Process の利用を廃止したいと考えています。

Copy link
Contributor Author

@omochi omochi left a comment

Choose a reason for hiding this comment

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

inline comment

@@ -0,0 +1,28 @@
import Foundation

public enum FileUtils {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CartonHelpersにおいてグローバル関数を散らかすのはよくないので、移動するついでにネームスペース型を作ります。

URL(fileURLWithPath: NSTemporaryDirectory())
}

public static func makeTemporaryFile(prefix: String, in directory: URL? = nil) throws -> URL {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

carton モジュールから引っ越しました。


private let processConcurrentQueue = DispatchQueue(
label: "carton.processConcurrentQueue",
attributes: .concurrent
Copy link
Contributor Author

Choose a reason for hiding this comment

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

コンカレントキューを使うことで並列性を得ます。
CartonHelpers.Process のやり方を真似しています。

Copy link
Member

Choose a reason for hiding this comment

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

TSC's Process already has output redirection as a stream.

outputRedirection: OutputRedirection = .collect, startNewProcessGroup: Bool = true,

Can we just use it?

return lines.joined(separator: "\n")
}

func checkSuccess() throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

他の箇所では nonZeroExit という言葉が使われていますが、
ProcessResult という文脈においては、意味的には success とした方が意味がわかりやすいと思います。

なお、 checkFoo 系のメソッドは 公式APIでは Task.checkCancellation() などが既存です。
これは Foo を期待し、そうでなければ例外を投げるというコンベンションで、それに合わせています。



extension Foundation.Process {
func waitUntilExit() async {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

async版を生やします。

}

func result(
output: Data? = nil,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Foundation.Process は結果を自分で持てないので、外から渡すインターフェースにしています。

struct SwiftRunProcess {
var process: Foundation.Process
var tee: Foundation.Process
var outputFile: AbsolutePath
Copy link
Contributor Author

Choose a reason for hiding this comment

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

teeコマンドをパイプで繋げるコンセプトとして型にまとめます。


func assertZeroExit(_ file: StaticString = #file, line: UInt = #line) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

元々のこのメソッドですが、命名と設計があまり良くないと思います。

まず命名ですが、 assertFoo は公式では、強い仮定を意味していて、失敗したらクラッシュします。
中身をみれば XCTAssert 系の動作をしますが、
その場合は XCTAssert... の名前にしないと、公式の assert と曖昧です。

また、ここをテストのアサーションにするのも無意味でしょう。
XCTestのアサーションは、失敗しても次の処理に進んで様々な値をテストするためのものですが、このようなテストでプロセスが失敗した後の続きの処理に意味はほとんどありません。

}
}

func swiftRunProcess(
_ arguments: [CustomStringConvertible],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swiftでは余計なexistentialで受ける事はあまりやらないと思います。
暗黙の型変換のように振る舞うので、呼び出し側で型がどうなってるのかわかりづらくなります。
よほどこれが活用されるようなパターンがあるわけでもなかったので、やめます。

["carton", "test", "--environment", "browser", "--port", "8082"],
packageDirectory: packageDirectory.url
)
XCTAssertTrue(result.stdout.contains(expectedContent))
XCTAssertTrue(try XCTUnwrap(result.utf8Output()).contains(expectedContent))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

キャプチャ結果に対する唯一のテストがこれです。

@omochi omochi changed the title ストリームしながらキャプチャするためにteeコマンドを使う コマンドの自動テストでストリームしながらキャプチャするためにteeコマンドを使う May 19, 2024
@omochi omochi marked this pull request as ready for review May 20, 2024 00:45
@kateinoigakukun
Copy link
Member

kateinoigakukun commented May 20, 2024

I personally prefer CartonHelpers.Process to Foundation.Process as much as possible because CartonHelpers.Process is already concurrency-ready and well-tested in TSC but Foundation.Process requires our own concurrency control.

However, I use Foundation.Process for places where we need to care about build time and don't want to introduce extra dependency. (e.g. in plugin code, carton executable)

In this case, we don't care about build time of test suites, so I prefer CartonHelpers.Process

@omochi
Copy link
Contributor Author

omochi commented May 20, 2024

まず端的に回答します。
了解です。
TSC.Process を使った実装に変更します。
ただ、frontendの将来の設計方針をどうすべきか意見を聞かせてください。
以下に詳細に返信します。


そうですね。
CartonHelpers.Process でこの課題にアプローチすれば、
必要なコード量は減ります。

また、 outputRedirection: に対して .stream を与えて、
そこから自分でコンソール出力とバッファリングを行うことができます。

However, I use Foundation.Process for places where we need to care about build time and don't want to introduce extra dependency. (e.g. in plugin code, carton executable)

はい、僕がこのアプローチを取ったのは、
すでに carton driver と plugin において Foundation.Process を使用していることから、
carton を swiftpm plugin として組み込んだ場合に、
エンドユーザにおける carton の利用時の carton 自体のビルド時間を短くしたいという意図を
まさにそのように推察したからです。

ただし、これを実現するためには、 carton frontend における CartonHelpers.Process の利用を廃止する必要があります。
plugin は frontend に依存しているため、frontend が CartonHelpers.Process を利用しているならば、
結局それのビルド時間が必要になってしまうからです。

また、 frontend は CartonHelpers に (CartonKit 経由で) 依存しているので、
利用を廃止するだけでなく、さらに、 CartonHelpers モジュールから CartonHelpers.Process を削除しなければなりません。

そこまで考えると、今回の実装に TSC.Process を使った場合、
最終的にテストコードだけから見える領域で、
CartonHelpers.Process を持っておくことになります。

テストよりも重要で複雑である driver, plugin, frontend が Foundation.Process だけで完結できるのであれば、
テスト用にわざわざ同じことができる TSC.Process を持ち込んで持っておくのはあまり意味がなさそうです。


なお、自前でやっている並行処理といっても、実態はかなり少ないです。

extension Foundation.Process {
  func waitUntilExit() async {
    await withCheckedContinuation { (continuation) in
      processConcurrentQueue.async {
        self.waitUntilExit()
        continuation.resume()
      }
    }
  }

これは DispatchQueue を使って同期処理を非同期にしただけのパターンです。

TSC.Process の実装は以下であり、ほぼ同じなので、正しいロジックだろうと思っています。

public final class Process {
  public func waitUntilExit() async throws -> ProcessResult {
    try await withCheckedThrowingContinuation { continuation in
      DispatchQueue.processConcurrent.async {
        self.waitUntilExit(continuation.resume(with:))
      }
    }
  }
}

TSC.Processposix_spawnDispatchGroup を使った並行処理を書いているため、
高度で複雑なコードになっています。
しかし、そういった難しさの大部分は Foundation.Process とそれに繋げる Foundation.Pipe
まさにすでに提供してくれているものです。
また、今回の実装ではストリームを2分岐する処理も tee プロセスに丸投げしているため、
そのデータフローにおいて特に自前の非同期プログラミングは必要ありません。

一方、 TSC.ProcessoutputRedirect は、
様々なスレッドやロックが噛み合ったコードの中からコールバックされる関数であり、
ある種その複雑さの中に巻き込まれることになります。

また実際のところ、 TSC.Process のWindows向けの実装は Foundation.Process をラップして利用しており、
それにより mac, Linux の実装よりもかなり少ないコードで書かれています。
このことは、 TSC.Process のコードがほとんど不要なものであることを示唆していると思います。


とはいえ、まだ frontend が TSC.Process を使っている段階で、
テストでそれを使わないようにする合理的な理由はないので、
あくまで先の形を見据えた設計ということになります。

手順としてはまず frontend から TSC.Process を排除するのが筋であるというのは尤もだと思うので、
今回のテストコードの変更は TSC.Process 上で構築するのはokです。

@omochi omochi changed the title コマンドの自動テストでストリームしながらキャプチャするためにteeコマンドを使う [DNM] コマンドの自動テストでストリームしながらキャプチャするためにteeコマンドを使う May 20, 2024
@kateinoigakukun
Copy link
Member

kateinoigakukun commented May 20, 2024

First of all, Foundation.Process is quite tricky compared with simple posix_spawn because it tries to emulate the darwin's Proceess behavior. For example, it implicitly closes all open descriptors unless explicitly declared, but it's not a default of posix_spawn and cannot be easily implemented on non-darwin platforms.

Thus I don't like to use Foundation.Process as much as possible.

ただ、frontendの将来の設計方針をどうすべきか意見を聞かせてください。

The eventual form in my mind would use the brand new Subprocess API from swift-foundation. So I felt migrating TSC.Process -> Foundation.Process -> Foundation.Subprocess requires boring work.

But given that the brand-new API is not going to be available soon, it might be worth using Foundation.Process for now here for simplicity.

@omochi
Copy link
Contributor Author

omochi commented May 20, 2024

なるほど、ありがとうございます。

Foundation.Process の実装も良くはないという事と、
Subprocess への移行を検討している事がわかりました。

確かに将来的にはこれがベストなため、
将来を見据えるならそれを考慮した方が良いですね。

@omochi
Copy link
Contributor Author

omochi commented May 20, 2024

#461 でやり直しました。

@omochi omochi closed this May 20, 2024
@omochi omochi deleted the tee branch May 20, 2024 22:05
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