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

Remove test utility ls #469

Merged
merged 2 commits into from
May 22, 2024
Merged

Conversation

omochi
Copy link
Contributor

@omochi omochi commented May 22, 2024

AbsolutePath.ls をしなくても、 FileSystem.getDirectoryContentsFileSystem.traverseRecursively があります。
テスト用のやり方を用意してもコード短縮の恩恵は少ないです。

また、 ls という名前なのに再帰的に中身を取り出す挙動は予測しづらいです。

必要ない道具は消したほうが、コードベースを把握しやすくなります。

また、 lsString を返しますが、
ここではファイルパスであることが確定しているので、
AbsolutePath を返すAPIの方が、
その先のデータ操作で様々なメソッドが使えて便利です。

使わなくても十分に整ったテストが書けるので消します。

#468 でもこの件に少し触れています。

XCTAssertTrue(fs.isDirectory(bundleDirectory), "The Bundle directory should exist")
XCTAssertTrue(bundleDirectory.ls().contains("index.html"), "Bundle does not have index.html")
XCTAssertFalse(
(bundleDirectory.ls().filter { $0.contains("wasm") }).isEmpty,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

元コードですが、
どちらも「含む」ことをテストしているのに
片方は contains + assert true、
片方は filter + is empty + assert false
と、一貫性がありませんでした。

まとめて書き直しました。

let files = try fs.traverseRecursively(bundleDirectory)

XCTAssertTrue(
files.contains { $0.basename == "index.html" },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらの API を使った方が、要素が AbsolutePath で取れるので、
そのさきのフィルタ処理を適切に書けます。

XCTFail("No wasm binary found")
return 0
}

return try localFileSystem.getFileInfo(bundleDirectory.appending(component: wasmFile)).size
return try localFileSystem.getFileInfo(wasmFile).size
Copy link
Contributor Author

Choose a reason for hiding this comment

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

すでに AbsolutePath なのでパスの組み立て処理が不要です。

@omochi
Copy link
Contributor Author

omochi commented May 22, 2024

<unknown>:0: error: -[CartonCommandTests.FrontendDevServerTests testDevServerPublish] : failed: caught error: "Error Domain=NSURLErrorDomain Code=-1004 "Could not connect to the server." UserInfo={_kCFStreamErrorCodeKey=61, NSUnderlyingError=0x600003c30420 {Error Domain=kCFErrorDomainCFNetwork Code=-1004 "(null)" UserInfo={_NSURLErrorNWPathKey=satisfied (Path is satisfied), viable, interface: lo0, _kCFStreamErrorCodeKey=61, _kCFStreamErrorDomainKey=1}}, _NSURLErrorFailingURLSessionTaskErrorKey=LocalDataTask <36571813-C7AF-4F98-B50E-5611A8A1AECD>.<3>, _NSURLErrorRelatedURLSessionTaskErrorKey=(
    "LocalDataTask <36571813-C7AF-4F98-B50E-5611A8A1AECD>.<3>"
), NSLocalizedDescription=Could not connect to the server., NSErrorFailingURLStringKey=http://127.0.0.1:8080/, NSErrorFailingURLKey=http://127.0.0.1:8080/, _kCFStreamErrorDomainKey=1}"
Test Case '-[CartonCommandTests.FrontendDevServerTests testDevServerPublish]' failed (106.212 seconds).
Test Suite 'FrontendDevServerTests' failed at 2024-05-22 14:02:15.653.
	 Executed 1 test, with 1 failure (1 unexpected) in 106.212 (106.212) seconds

起動まちが3秒じゃ足りないか?

@omochi
Copy link
Contributor Author

omochi commented May 22, 2024

それの修正はまた別でやろう
いったんリトライ

@omochi omochi changed the title テストユーティリティの ls を削除する [1/2] テストユーティリティの ls を削除する May 22, 2024
@kateinoigakukun kateinoigakukun changed the title [1/2] テストユーティリティの ls を削除する Remove test utility ls May 22, 2024
@kateinoigakukun kateinoigakukun merged commit b26110b into swiftwasm:main May 22, 2024
4 checks passed
@omochi omochi deleted the remove-ls-in-test branch May 22, 2024 22:14
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