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

Grepテストを導入する #1660

Closed

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented May 6, 2021

PR の目的

Grep機能に自動テストを導入します。
そろそろ2週間くらい放置してるので、導入案を提示しておきます。

カテゴリ

  • 実験 (master へのマージを目的としない)

PR の背景

Grep機能はサクラエディタの目玉機能の1つです。

重要な機能なので、保守性や信頼性を高めていきたいと考えていますが、
テストがないので手を加えることができません。

PR のメリット

PR のデメリット (トレードオフとかあれば)

  • メイン目的のテストコードが、かなり高度です。
    誰もレビューできない可能性があります。
  • メイン目的を実現させるための前提となる修正が、めちゃめちゃ高度です。
    全体のレビューは誰もできないと思います。
  • ぶっちゃけ、未完成です。
    MinGWビルドの出力フォルダにbregonig.dllをコピーする処理をまだ書いていません。

仕様・動作説明

現時点で細かいことを説明しても
「何それ、おいしい?」
だと思うのでは説明しません。

PR の影響範囲

サクラエディタの機能に影響を与えるPRではありません。
本体のビルド方式に影響を与えます。
テストのビルド方式に影響を与えます。
テストの実行時間がめちゃめちゃ長くなります。

テスト内容

このPRはテストを追加します。

確認しているのはビルド&tests1.exe実行のみで、現状MinGWビルドのテストは失敗します。

関連 issue, PR

参考資料

@AppVeyorBot
Copy link

Build sakura 1.0.3734 failed (commit f59fd5c9aa by @berryzplus)

tests1.exeに英語リソースを組み込むのが面倒なのでリネームして無効化する。
@AppVeyorBot
Copy link

Build sakura 1.0.3735 failed (commit 8889a743f7 by @berryzplus)

@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

CIエラーの原因

  1. tests1.exeに英語リソースを含めていなかった。
    👉 とりえあず対応済み。
  2. Azure PipelinesでCZipFileが機能しないっぽい。
    実行環境の言語リソース(英語)を含めてもエラーになる。
    zipフォルダは 正規 Windows 推奨プログラム の特典だったような気がしていて、dockerコンテナやWine等の疑似Windows環境では機能しない可能性については、元々ちょっとだけ懸念していました。
    👉 Windows正規品判定の偽陰性(?)対策は面倒なので、環境変数を見てAzure Pipelines特有のエントリがあったらスキップしてしまおうかと思っています。
  3. SonarCloud設定の変更は、マージ後でないと反映されないっぽい。
    👉 この件はfatalなのでPRを分割して対応します。
    分割してしまうと、最終目的に至る手段がボヤけるのを嫌って一括としてました。

@berryzplus
Copy link
Contributor Author

ん?CZipFIleテストの失敗原因は違うかもしれない。。。

@berryzplus
Copy link
Contributor Author

やれる限りで追加のCIエラー対策を入れてみました。

@berryzplus
Copy link
Contributor Author

コンパイル通っていませんでした。

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

Build sakura 1.0.3738 failed (commit 22a158305e by @berryzplus)

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

Build sakura 1.0.3741 failed (commit ab8e5fe132 by @berryzplus)

@AppVeyorBot
Copy link

@sanomari
Copy link
Contributor

sanomari commented May 8, 2021

どのあたりが高度なんですかね?(笑

修正の効果をイメージしづらい変更が多いように見えました。
このPRで最終刑をイメージできると思うので、細かい別件は分けたらよいと思います。

テストリソースをzipで添付するのは良いアイディアだと思いました。
ただし、レポジトリに登録するのは圧縮前のファイルとしたほうが良いと思います。

あと、Grep対象ファイルは320,000行くらい(32×10,000)にしてやれば中止ボタン押下チェックのロジックが動かせるんじゃないかと覆いました。

@berryzplus
Copy link
Contributor Author

どのあたりが高度なんですかね?(笑

よく分かりませんが「このくらいのレベル」のPRが通ったことはほとんどないです。

ただし、レポジトリに登録するのは圧縮前のファイルとしたほうが良いと思います。

あと、Grep対象ファイルは320,000行くらい(32×10,000)にしてやれば中止ボタン押下チェックのロジックが動かせるんじゃないかと覆いました。

圧縮前のテストリソースを登録するのは良さそうです。
Grep対象ファイルのデータを変えると結果を変えないといかんのでややめんどいですが 😄

@sanomari
Copy link
Contributor

sanomari commented May 9, 2021

テストパターンを簡略化できるように思います。

  • ファイル検索 9ケース
    • ファイル検索機能 共通で1パターン
    • 結果出力機能 9ケース
  • Grep検索 4ケース
    • Grep検索機能 共通で1パターン
    • 結果出力機能 4ケース
  • Grep置換 テスト結果の評価に利用
    • Grep置換機能 13パターン
    • 結果出力機能 結果出力の検証はなし

構造的に結果出力機能はグローバル関数に抽出できそうなので、時間のかかるテストは3つやれば足りるように思います。

@sonarcloud
Copy link

sonarcloud bot commented May 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@berryzplus
Copy link
Contributor Author

ぬぅ、コンフリクトしてしまった 😃

@berryzplus
Copy link
Contributor Author

#1670 のマージにより、Grepテストの導入に向けての前提条件が整います。

リベースして続行すると分かりにくいと思うので、こちらのPRは閉じてしまいます。

@berryzplus berryzplus closed this May 20, 2021
@berryzplus berryzplus deleted the feature/test_of_grepstdout branch May 20, 2021 14:55
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.

3 participants