Go の Short variable declarations と Named return values
go-mp4という、mp4ファイルをパースしてくれるGoで書かれたライブラリがあった。
Goに慣れるため、golintのカバレッジを上げるPRを出してみた。
自分の書いたコードで、:=ではno new variables on left side of :=というエラーが出たけれど、代わりに=を使えばうまくいく、という事象があった。
具体的には、以下の部分でエラーが出ていた。
func UnmarshalAny(r io.ReadSeeker, boxType BoxType, payloadSize uint64, ctx Context) (box IBox, n uint64, err error) { dst, err := boxType.New(ctx) if err != nil { return nil, 0, err } n, err := Unmarshal(r, payloadSize, dst, ctx) // no new variables on left side of := return dst, n, err }
Goの言語仕様のShort_variable_declarationsによると、同じブロック内で同じ型として宣言された変数に対して、宣言する変数のうち少なくともひとつはブランク変数でなければ、再宣言が可能ということだった。
Unlike regular variable declarations, a short variable declaration may redeclare variables provided they were originally declared earlier in the same block (or the parameter lists if the block is the function body) with the same type, and at least one of the non-blank variables is new.
ただし今回は、dst, err := boxType.New(ctx)に対する再宣言が原因でエラーが出ているわけではない。
以下のコードがコンパイルエラーを起こさないことがそれを証明している。
(本来dstはIBoxという型だが、ここではstringにしている。)
func main() { dst, err := "foo", fmt.Errorf("error1") n, err := 1, fmt.Errorf("error2") fmt.Println(dst,n,err) // foo 1 error2 }
試しに以下のように、関数内で使われているnという変数名をn1に変えてみると、no new variables on left side of :=というエラーは起こらなかった。
func UnmarshalAny(r io.ReadSeeker, boxType BoxType, payloadSize uint64, ctx Context) (box IBox, n uint64, err error) { dst, err := boxType.New(ctx) if err != nil { return nil, 0, err } // n1に変えた n1, err := Unmarshal(r, payloadSize, dst, ctx) return dst, n1, err }
また、戻り値の変数の名前(named return value)をnからn1に変えてみても、no new variables on left side of :=というエラーは起こらなかった。
func UnmarshalAny(r io.ReadSeeker, boxType BoxType, payloadSize uint64, ctx Context) (box IBox, n1 uint64, err error) { // n1に変えた dst, err := boxType.New(ctx) if err != nil { return nil, 0, err } n, err := Unmarshal(r, payloadSize, dst, ctx) return dst, n, err }
A Tour of Go の Named return values によると、nとerrorは関数の最初で定義した変数名として扱われるということだった。
Goでの戻り値となる変数に名前をつける( named return value )ことができます。戻り値に名前をつけると、関数の最初で定義した変数名として扱われます。
この戻り値の名前は、戻り値の意味を示す名前とすることで、関数のドキュメントとして表現するようにしましょう。
つまり、以下のコードが通らず、
func main() { var n uint64 var err error n, err := 1, fmt.Errorf("error") // no new variables on left side of := }
以下のコードが通る、ということが今回起きていたのだった。
func main() { var n uint64 var err error n, err = 1, fmt.Errorf("error") }
Goの言語仕様でnamed return valueの仕様は、さっとみた限りヒットしなかったが、Defer statementsの欄に、
function has named result parameters that are in scope within the literal, the deferred function may access and modify the result parameters before they are returned.
と書いてあった。
エラーを報告する際に意識していること
基本的には質問をせずに粘ってしまうタイプなのだが、環境構築の段階でのエラーや、人生で初見のエラーなどは経験上、粘っても何も得られないので、即質問するようにしている。
そこで意識していることは、
- 何が原因で何ができなかったのかを記す
- 自分が打ったコマンドのログを全て記す
- なぜそのコマンドを打ったのかの情報源を記す
- その結果のログを記す
- 自分なりの解決案があれば記す
具体例
github.com
qrcode.react とスナップショットテストの相性が最高すぎた
qrcode.reactは、その名の通りQRコードを生成してくれるReactコンポーネントだ。
github.com
ファイル構成はsrc/index.jsでゴリゴリ計算というか文字列を算出し、コンポーネントを返す構成だった。
テストがなかったので、テストを追加したかったのだが、何に対してテストを追加すればよいのかわからず、しばらく放置していた。
convertStr関数やgeneratePath関数の入力と出力に対する単体テストを追加しようと思ったけれど、あまり恩恵はないように思えた。
qrcode.react/index.js at 6aeb42abc26ffecc868b630b6ad8f507d2125813 · zpao/qrcode.react · GitHub
qrcode.react/index.js at 6aeb42abc26ffecc868b630b6ad8f507d2125813 · zpao/qrcode.react · GitHub
そんなとき、以下の記事を目にした。
www.mizdra.net
試しにqrcode.reactで試してみると、とてもよい結果が得られた。
何がよかったかと思うと、convertStr関数やgeneratePath関数の入力と出力に対する単体テストをせずとも、それらがうまく機能していることを保証でき、なにより、pathコンポーネントのdの値の複雑性がスナップショットの出力により明らかになり、スナップショットの結果が1つの情報としても機能しているところだ。
<path
d="M0,0 h29v29H0z"
fill="#ffffff"
/>
<path
d="M0 0h7v1H0zM8 0h1v1H8zM10 0h5v1H10zM17 0h2v1H17zM22,0 h7v1H22zM0 1h1v1H0zM6 1h1v1H6zM9 1h2v1H9zM13 1h1v1H13zM19 ....
fill="#000000"
/>マージされなくてもブログを書けば供養できると思って書いた。
npmから@typesを使う場合はDefinitelyTypedを見に行かないといけないことがある
Link Preview JSという、「allows you to extract information from a HTTP url/link (or parse a HTML document) and retrieve meta information such as title, description, images, videos, etc」なレポジトリがあった。
そこでは、cheerioというライブラリを用いて、パースされたレスポンスから特定の要素を取得している。
https://github.com/ospfranco/link-preview-js/blob/d22888f26718674d7a32cfb935434f7357c80817/index.ts#L236
Link Preview JSはTypeScriptで書かれているのだが、cheerioに対する型が全くなく、anyが使用されていたので、@types/cheerioを導入した。
だが、一部だけ以下のようなエラーがでて、@types/cheerioを用いてもanyが残ってしまっていた。
Property 'attribs' does not exist on type 'Element'. Property 'attribs' does not exist on type 'TextElement'. ts(2339)
https://github.com/ospfranco/link-preview-js/pull/83#discussion_r563268635
このまま放置するのも気持ちが悪かったので、実際に@types/cherrioの型が書かれているDefinitely Typedをみにいった。
github.com
DefinitelyTyped/types/cheerio at master · DefinitelyTyped/DefinitelyTyped · GitHub
すると、cheerio.ElementのTagElementにのみattribsの型が定義されていることがわかった。
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/68e48c5ad46f0e7733feace0a9120d7dc3108462/types/cheerio/index.d.ts#L33
テストを見ても、elementのtypeがtextではない、つまり、TextElementではなくTagElementであることを確認している。
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b41bbafcd298b4b2aaa851d43ffadb6ef3565e69/types/cheerio/cheerio-tests.ts#L347
なので、それを適用して、自分で作ったanyを回収しにいった。
github.com
src/@types/*d.tsに型定義があるのと比べて、npmでインストールした場合は、node_modules/@types/*/*d.tsに型定義が隠蔽されてしまうので、便利だけど難しいなと思う。
それを解消する何かがもうすでにあるのだろうか。
TwitterでこのOSSの作者から感謝してもらえた。
https://twitter.com/ospfranco/status/1353312260823855105
(フォローしたけどフォローは返ってこなかった)
自分のPRを見て思う、TableDrivenTestsの良さ
jdという、commandline utility and Go library for diffing and patching JSON valuesなライブラリにTableDrivenTestsを導入したので、その良さを主観的にまとめておこうと思う。
まずは、実際にdiffの一部を見てみる。
https://github.com/josephburnett/jd/pull/29/files#diff-a0a78ab7b3426f60e7b758b565129777bdcef35e95a490999a0ff377389576d1R83
before
ctx := newTestContext(t) checkDiff(ctx, `[]`, `[]`) checkDiff(ctx, `[[]]`, `[[1]]`, `@ [0,0]`, `+ 1`) checkDiff(ctx, `[1,2,3]`, `[1,null,3]`, `@ [1]`, `- 2`, `+ null`) checkDiff(ctx, `[]`, `[3,4,5]`, `@ [0]`, `+ 3`, `@ [1]`, `+ 4`, `@ [2]`, `+ 5`)
after
ctx := newTestContext(t) tests := []struct { context *testContext a string b string diffLines []string }{ {ctx, `[]`, `[]`, []string {}}, {ctx, `[[]]`, `[[1]]`, []string { `@ [0,0]`, `+ 1`, }, }, {ctx, `[1,2,3]`, `[1,null,3]`, []string { `@ [1]`, `- 2`, `+ null`, }, }, {ctx, `[]`, `[3,4,5]`, []string { `@ [0]`, `+ 3`, `@ [1]`, `+ 4`, `@ [2]`, `+ 5`, }, }, } for _, tt := range tests { checkDiff(tt.context, tt.a, tt.b, tt.diffLines...) }
よいと思うところを2つ書く。
関数呼び出しが冗長でなくなった
beforeとafterで関数を呼び出す回数は変わらないが、afterはループの中で呼び出しているので、パッと見て冗長な表現にはなっていない。
func TestArrayDiff(t *testing.T) {のなかのテストなので、checkDiffを何回も呼び出している(ように見える)のは多少くどい。
関数の引数が宣言的になった
checkDiff関数は、func checkDiff(ctx *testContext, a, b string, diffLines ...string) {という形で引数を取る。
jd/common_test.go at d1d4edfee5375f88c1155ef55158597fb8488751 · josephburnett/jd · GitHub
beforeでは、どこからがdiffLinesが多少わかりにくくなっているが、afterでは、[]stringでまとめているため、checkDiffに何がどの引数で渡されるのかが分かりやすくなっている。
{
context *testContext
a string
b string
diffLines []string
}
また、上のように、テストのはじめに宣言できることで、可読性が非常に高くなっている。
今回のようにbeforeがあの状態だったら、TableDrivenTestを導入しない手はないのではないかと思う。
割といい変更がだせたので自画自賛タイトルをつけてみた。
単純作業の効用は意外と大きい
owという、TypeScriptの型制限を拡張したOSSに何度かコミットして気づいたことは、単純作業は意外と効用が大きいということだ。
github.com
具体的に何をしたかというと、typescript-eslintの、explicit-function-return-typeが無効になっていたのを、有効にした。
github.com
まずはじめにpackage.jsonのexplicit-function-return-typeの設定を"off"から"error"に変更し、テストを走らせた。
https://github.com/sindresorhus/ow/pull/206/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R89
あとは、そのテストが落ちた箇所にひたすら型をつけていった。
この型づけは自分的には単純作業のうちに入るのだが、自分のスキルはそれほど伸びないものの、得られる効用は大きい。
ほんの30分の型づけにより、これからこのOSSにコミットする全ての人は、function return type をつけなければならなくなった(つけないとテストが落ちる)。
これは、このOSSの保守性に直結することである。
関数が返す型が明確になると、初めてこのOSSに訪れる人のコードリーディングが捗るというオプションもついてくる。
このように、O(1)の単純作業でO(N)の効用が得られる。
ただし、個人的にはこのexplicit-function-return-typeを有効にするには然るべきタイミングがあると思っている。
まず、プロジェクトの初期からこれを有効にすると、テストが高確率で落ちることになり、開発スピードが落ちてしまう。
また、explicit-function-return-typeを有効にするときに、他のPRが溜まっている状態だったら、凄まじいコンフリクトが生じてしまう。
https://github.com/sindresorhus/ow/commits/main
なので、今回は、OSSとしてある程度枯れていること、他の人とPRが被らない時期であることを確認してから、このPRを出した。
ちなみにこういった単純作業をするときは、ミスチルの彩りを聴きながらすると捗る。
Cypress で `blitz new` をテストしたい(が、できていない)
ことの発端は、最近よくBlitzを聞くようになり、GitHubを訪れると、good first issueが転がっていて、テストに関するものがあったので、Blitzを触ったことも、GraphQLやPrismaもあまり知らなかったけど、拾ってみた。
このissueの内容は、blitz newの挙動をCypressを使って確かめたい、というものだった。
Add e2e CLI test for `blitz new` · Issue #37 · blitz-js/legacy-framework · GitHub
We currently have a few blitz new tests, but not a complete e2e test for the new app.
...
Add Cypress to our CLI testing setup. See the Redwood CLI cypress test for a great example/starting point.
Cypressもはじめて聞く名前だったが、ドキュメントを読んでちょっと手元で動かして何となく理解し、Blitzに導入することにした。
(Cypress導入第1人者かと思っていたが、先人がいた)
マージされてコントリビューターにもなれたので、本腰をいれてテストを増やしていこうと思ったのだが、Blitzレポジトリの中でblitz newをe2eテストする、ということがよくわからなくなってしまった。
blitzを試すために手元でblitz newしたレポジトリも作って触っていて、どっちで何をしたいのかわからなくなり混乱が加速した。
なので、issueのなかで言及されていた、Redwoodのe2eテストがどうなっているのかを見にいくことにした。
まずは、RedwoodのCONTRIBUTING.mdを読み、e2eテストを動かすことにした。
が、うまく動かなかったのでissueを出した。
すると、2日足らずで問題は解決したようで、Cypressに慣れるためにも新たにテストを追加した。
Cypressは完全に理解できたので、Blitzにも入れていきたいが、Blitzレポジトリの中でblitz newをe2eテストするということがよくわからい問題が健在している。
Redwoodでは、起点がシェルスクリプトで、"We will copy './packages/create-redwood-app/template' and the packages/*"していることがわかっている。
redwood/test-tutorial at d61734296bf843d50933d36cabb2a9ea25ede919 · redwoodjs/redwood · GitHub
だが、Blitzに'./packages/create-redwood-app/template'に当たるものがあるとは思えない。
また、これと同じようにシェルスクリプトで起動するとしても、Blitzの場合はデフォルトのフォームを選択する部分があり、そこをどうモックしたらよいのかわからない問題もある。
blitz/new.ts at e2383fbb5f52a59042640dcf4cb67d2c8344de06 · blitz-js/blitz · GitHub
Blitz既存のnew.test.tsを見にいったところ、mockImplementationでモックしていることがわかったが、これはあくまでcliのテストなのでやりたいこととは違う。
jest.mock("enquirer", () => { return jest.fn().mockImplementation(() => { return { prompt: jest.fn().mockImplementation(() => ({form: "React Final Form"})), } }) })
詰まってしまったので、そもそもblitz newコマンドを打ったら、どのようにしてファイル群を生成しているのかを見に行ったら、ヒントがあるのではないかと思い読み始めた。
すると、new.tsでは、generatorを生成してそれを走らせているっぽいことがわかった。
blitz/new.ts at e2383fbb5f52a59042640dcf4cb67d2c8344de06 · blitz-js/blitz · GitHub
blitz/new.ts at e2383fbb5f52a59042640dcf4cb67d2c8344de06 · blitz-js/blitz · GitHub
const generator = new (require("@blitzjs/generator").AppGenerator)({ ... await generator.run()
blitz newのテストは実質的にはgeneratorのテストなのか？、ということで"@blitzjs/generator"を読みにいこうとしたところで力尽きている。
exmapleにすでにテストがあるのでそれでいいのではないかと思い始めているけど、勉強になりそうなのでちょっとづつ読んでいきたいと思っている。
https://github.com/blitz-js/blitz/blob/canary/examples/auth/cypress/integration/index.test.ts
2/19追記
was this issue resolved in #1846?
という質問が来ていたので、疑問点をissueに書き込んでおいた。
https://github.com/blitz-js/blitz/issues/1767#issuecomment-781707012
