Skip to content
This repository has been archived by the owner on Apr 12, 2023. It is now read-only.

Debug_Mockの機能拡張(高リスク接触等) #179

Closed
i-maruyama opened this issue May 14, 2021 · 17 comments · Fixed by #191
Closed

Debug_Mockの機能拡張(高リスク接触等) #179

i-maruyama opened this issue May 14, 2021 · 17 comments · Fixed by #191
Labels
confirmed 開発内部管理用 enhancement 新しい機能や改善のリクエスト

Comments

@i-maruyama
Copy link
Contributor

i-maruyama commented May 14, 2021

その機能リクエストは何らかの問題に関連しますか / Is your feature request related to a problem?

  • 現在のDebug_Mock ビルドでは、高リスク接触がテストできない問題(現状は低リスク接触2件のみ)
  • 高リスクの接触をテストするにはデバッガで変数を書き換えればよいが、面倒な問題
  • Debug_Mock では、本物の TeKダウンロードが出来ない問題

解決策についてお書きください / Describe the solution you'd like

  • TestNativeImplementation.cs と HttpDataServiceMock.cs を改変し高リスクの接触を検出させる
  • 既定のデフォルト状況セットを用意しておき、 settings.json の値だけ変えれば状況を変えて簡単にビルド出来るようにする( Debug_Mock ビルドでは settings.json のほとんどを使っていない)
  • DebugPage DebugPage の追加 #160 には、 EN 停止、LastProcessTekTimestamp リセットなどがあり、これを用いてリアルタイムの状況変更が出来るようにする。(再ビルドしなくてもテスト出来るように)
  • settings.json に デフォルト以外のURL が指定されたときは、TEKダウンロードを行う。

あなたが考える代替案についてご説明ください / Describe alternatives you've considered

  • Debug_Mock ビルドの単体テストが通らないときなどは、開発チームに影響しないよう、ビルド名を変えるなどもありうる
  • TEK配信サーバーを立てて、ダウンロードファイルに対応する複雑な挙動が可能になるよう TestNativeImplementation.cs に実装するという壮大な事もありうる(やりたい人は別 issue で)
  • 陽性登録の成功失敗 (DebugPage の追加 #160 (comment)) もあっても良い。

その他 / Additional context

いくつかは #160 での議論の中で実験しています。まだ、バックグラウンドでの接触通知が COCOA から来るまでには至っていません。


Internal IDs:

  • NFR 2581
@tatsu-jp
Copy link
Contributor

はじめまして、tatsuです。GW頃からcocoaコードを見始めて色々と試していた初学者です。

本提案と少しズレるかもしれませんが、Device_Mockのデフォルト値を接触有りにした方がプロジェクト参加者には良いと思います。
初学者にとって接触有り判定を出せるようにするのは大変だと思いますが、接触有り判定から無し判定へ変更するのは前者よりは容易であると考えるためです。

私の考える解決策としては、以下の2つを修正することです。

  • TestNativeImprementation.csにEN API戻り値(接触判定される値)を追加
    →リスクスコアがMinimumRick(21)より大きい値を入れています。
  • HttpDataServicesMock.csにGetTemporaryExposureKeyList()の戻り値のダミーを追加
    →TEKキーをダウンロードしてもMockでは処理できない(EN APIを呼び出せない)ので、ダミーを返すようにしています。

また、「陽性情報の登録機能」は以下の問題で実行完了しません。

  • Android/iOSシミュレータではDevice Verificationできない
    →Device Verificationを回避する手順(L392をコメントアウト)をまとめるのが良いかなと思っています。MockかどうかをBooleanで返すメソッドを用意し、Mockの場合はVerificationしないようにすれば動的に切り替えられます。ただ、本番環境へ影響ある修正方法しか思いついていません(本番に影響ない形で実装する方法あれば良いですが…)。Cocoa開発メンバーがBooleanの判定結果に応じて振る舞いを切り替える修正を受け入れられるのであれば、後者の方がDebug_Mock利用者にとっては助かります。

// See if we can add the device verification
if (DependencyService.Get<IDeviceVerifier>() is IDeviceVerifier verifier)
{
submission.DeviceVerificationPayload = await verifier?.VerifyAsync(submission);
}

  • HTTPDataServieMock:PutSelfExposureKeysAsyncの戻り値が呼び出し側の期待値では無いため、登録失敗
    
→デフォルトの戻り値は正常系の方が望ましいと思いますので、修正させてください。

加えて、iOSシミュレータではSecureStorageにアクセスできず(※)、接触記録をストレージに保存することができないため、接触確認周りのView遷移をテストできません。
Apple開発環境を持っている方には問題ないと思われますが、そこまでせずとも利用できるようにSecureStorageMockを追加させていただきたいと思います(デフォルト無効)。

※Secure Storageにアクセスするにはプロビジョニングが必要だが、Xamarin.iOS アプリの無料プロビジョニングによると、配布済みのアプリでは無料プロビジョニングができない。Apple Developer Program(有償)に参加すれば可能だと思われる(未検証)。Androidシミュレータではこの問題は発生しない。

以上を実装したサンプルコードが以下になります(Device Verificationの提案は含んでいません)。
https://github.com/tatsu-jp/cocoa/commit/1e189eb4a4a7ee99d76ec23849bbe805341de587

上記に対応することで、DeviceMockで一通りのVeiw遷移をテストできるのではないかと思います。

@i-maruyama
Copy link
Contributor Author

はじめまして。参入は私もGW前ぐらいです。おっしゃるようにすると陽性確認でると思いますが、バックグラウンドでの接触通知が COCOA から来ましたか?私はまだ成功していません。

Device_Mockのデフォルト値を接触有りにした方がプロジェクト参加者には良いと思います。

デフォルト値については、 UnitTest が関与するかもしれないので(確認はしていません)、私は変えていません。それより良い案を今エンコードしています。

Debug_Mock では settings.json をほとんど使っていないのですが、これを使ってデフォルト値を設定する案です。
例えば、ApiUrlBase なら

  • ApiUrlBase = "https://API_URL_BASE/api/" -> 現行デフォルト値(UnitTest では、これが必要)
  • ApiUrlBase = "https://API_URL_BASE/api/1" -> 接触ありの設定
  • ApiUrlBase = "10,2,5,0,0,10,15,65,5,4,11,5,40,3,2" -> 現行デフォルト値と同一だが、より一般に数値でも指定可能
    という方法です。

こうしておくと一般ユーザーには、settings.json を変更して Debug_Mock ビルドしてもらえば複数の状況をテストしてもらえることになります。TeKダウンロード系は、 CdnUrlBaseで同様な形です。

一方で、「陽性情報の登録機能」は、手を出していません。また、iOS用 secureStrageMock は、ぜひ別 issue で立てられたらどうでしょうか?少なくとも私は iOS の環境がありません。

@tatsu-jp
Copy link
Contributor

コメントありがとうございます。
(すみません、Device_MockはDebug_Mockの間違いです。。。)

接触通知はOSの通知のことですかね?私はDebug_Mockでしか確認していないので、接触通知については確認していません。
接触通知を出すにはDebug/Releaseで実行してEN APIを呼び出す必要があると思います。挙動に関しては #17 (comment) が参考になるのではないかと思います。

Debug_Mockで以下を無効にしてNative側の実装を動かしているかもですが、EN APIを呼び出すには許可が必要なので、通知を受け取るのは無理なのではないかと思います。

public static void UseMockExposureNotificationImplementationIfNeeded()
{
#if USE_MOCK
// For debug mode, set the mock api provider to interact
// with some fake data
Xamarin.ExposureNotifications.ExposureNotification.OverrideNativeImplementation(new Services.TestNativeImplementation());
#endif
}

デフォルト値については、 UnitTest が関与するかもしれないので(確認はしていません)

TestNativeImplementationをnewしているのはApp.xaml.cs内だけという認識ですが、私の見逃しもあるかもしれません。
この方法でなくても、wiki等にEN API戻り値に関する仕様と追加方法がまとまっていれば十分だとも思います。

接触情報等をファイルで記載して読み込むのは良い案だと思います。ただ、既存のパラメータを異なる用途で利用するのは後から見た時に混乱しそうですので、別ファイルに分けて記載した方が良いと思います。

一方で、「陽性情報の登録機能」は、手を出していません。また、iOS用 secureStrageMock は、ぜひ別 issue で立てられたらどうでしょうか?少なくとも私は iOS の環境がありません。

提案内容とは異なるので分離した方が良いかもですね。
@keiji さん、以下の2つのissueを起票しようと思いますがチケット内容や粒度として適切でしょうか。コメント頂けると助かります。

  • Debug_Mockにおける陽性情報の登録に関する問題
  • Debug_MockにおけるiOS secureStorageに関する問題

@keiji
Copy link
Collaborator

keiji commented May 15, 2021

iOSシミュレーターでSecureStorageが利用できない(SecureStorageMockの作成)

みたいな感じでしょうか。

Debug_Mockにおける陽性情報の登録に関する問題

こちらまだぼくがちゃんと理解できてないので詳しく教えてください。

現時点で「陽性登録を成功させるパターンをテストしたい」という要望と、「接触通知を表示させてテストしたい」という要望の二つあるように思います。

「陽性情報の登録に関する問題」は、後者に関係したIssueになりそうでしょうか。

@tatsu-jp
Copy link
Contributor

tatsu-jp commented May 15, 2021

コメントありがとうございます。
上記で言いたいのは「陽性登録を成功させるパターンをテストしたい」の方です。HOME画面の「陽性情報の登録」機能を指しています。
(追記) それほど優先度の高いissueではないので、発行しなくても大丈夫です。

SecureStorageに関しては後でissue発行します。

@i-maruyama
Copy link
Contributor Author

EN APIを呼び出すには許可が必要なので、通知を受け取るのは無理なのではないかと思います。

情報ありがとうございます。(通知を楽しみにしていたので残念)

別ファイルに分けて記載した方が良い

もし、PR が通ったら、ドキュメントも記載したいです(Wikiかな?)。順にテストしていくと沢山接触履歴が出て楽しめて、かつ、COCOAの機能がAPIによる接触確認とTEKダウンロードの二つにある事が、わかってもらえるようなものが理想です。

@keiji keiji added the enhancement 新しい機能や改善のリクエスト label May 15, 2021
@i-maruyama
Copy link
Contributor Author

i-maruyama@1847252 にコミットしています。(こういう表示のさせ方できるのを知りませんでした)

このコミットと、#178 を合わせていただいて、
settings.json の中の二つを以下のように変更し、

  "apiUrlBase": "https://API_URL_BASE/api/1",     // 高リスク接触1+低リスク接触2
  "cdnUrlBase": "https://CDN_URL_BASE/1",         // リアルタイムTEK

とすると、FetchExposureKeyAsync するたびに、陽性者との接触が増えていきます。

debugmock

ちょっと、setting.json の文字列指定の仕方と、事前データセットについては、議論の余地があると思っています。

今は、デフォルト値から単に1を追記すればよいように

  "apiUrlBase": "https://API_URL_BASE/api1",     // 高リスク接触1+低リスク接触2

という文字列に変更したいと思っています。

EN 停止によってリアルタイムに接触確認結果を変更するのは、今は実装していません。これをするなら、 DebugPage にボタンを出現させればよい、という考えになりました。もしそうするなら、今の DebugPage の PR が決着してからにしたいと思います。

@i-maruyama
Copy link
Contributor Author

i-maruyama commented May 15, 2021

https://github.com/tatsu-jp/cocoa/commit/1e189eb4a4a7ee99d76ec23849bbe805341de587 も拝見しまして、だいたいやりたいことは包含されていると思うのですが、
そこでの現行72行目の修正

-                return HttpStatusCode.OK;
+                return HttpStatusCode.NoContent;

は、理解できておらず、取りこぼしています。

@tatsu-jp
Copy link
Contributor

プロジェクト参加者がDebug_Mockの制限事項を理解して、Debug_Mockで各種テストを簡単にできるようになると良いですね。

陽性者登録に関しては、以下のHttpDataService.PutSelfExposureKeysAsync(selfDiag)で実行します。
Mockのメソッドの戻り値を変更することで各ダイアログとメッセージのテストを実行できると思います。

ステータス処理を見ていただくと分かると思いますが、"OK"はUnexpected statusに該当します(意図的なのかもしれませんが)。

var selfDiag = await CreateSubmissionAsync(temporaryExposureKeys, positiveDiagnosis);
HttpStatusCode httpStatusCode = await HttpDataService.PutSelfExposureKeysAsync(selfDiag);
loggerService.Info($"HTTP status is {httpStatusCode}({(int)httpStatusCode}).");
switch (httpStatusCode)
{
case HttpStatusCode.NoContent:
// Success
loggerService.Info("Success send of diagnosis number");
break;
case HttpStatusCode.NotAcceptable:
await UserDialogs.Instance.AlertAsync(
"",
AppResources.ExposureNotificationHandler1ErrorMessage,
AppResources.ButtonOk);
loggerService.Error($"The diagnostic number is incorrect.");
throw new InvalidOperationException();
case HttpStatusCode.InternalServerError:
case HttpStatusCode.ServiceUnavailable:
await UserDialogs.Instance.AlertAsync(
"",
AppResources.ExposureNotificationHandler2ErrorMessage,
AppResources.ButtonOk);
loggerService.Error($"Cannot connect to the center.");
throw new InvalidOperationException();
case HttpStatusCode.BadRequest:
await UserDialogs.Instance.AlertAsync(
"",
AppResources.ExposureNotificationHandler3ErrorMessage,
Resources.AppResources.ButtonOk);
loggerService.Error($"There is a problem with the record data.");
throw new InvalidOperationException();
default:
loggerService.Error($"Unexpected status");
throw new Exception("Unexpected status");
}
}
finally
{
exposureNotificationService.PositiveDiagnosis = null;
loggerService.EndMethod();
}

@i-maruyama
Copy link
Contributor Author

おかげで理解できました。ありがとうございます。

デバッグという意味からすると、ステータスコードを全部網羅出来たらいいのかもしれませんね。

ApiUrlBase は、PostRegisterUserAsync の初期登録(url/register)とPutSelfExposureKeysAsyncの陽性登録(url/v2/diagnosis)で使われていますので、

"apiUrlBase": "https://API_URL_BASE/api1/register1/diagnosis1",    

こんな感じで設定できるようにしようと思います。

register0(default) ユーザー登録成功
register1 ユーザー登録失敗


diagnosis0(default) HttpStatusCode.OK 陽性登録失敗
diagnosis1 HttpStatusCode.NoContent 陽性登録成功
...
diagnosis100 百を超える数は、該当のHttpStatusCodeに変換して返す。(100は HttpStatusCode.Continue です)

@i-maruyama
Copy link
Contributor Author

i-maruyama commented May 16, 2021

とりあえず、上記の方針で作りました。

問題の以下の部分ですが、

// See if we can add the device verification
if (DependencyService.Get<IDeviceVerifier>() is IDeviceVerifier verifier)
{
submission.DeviceVerificationPayload = await verifier?.VerifyAsync(submission);
}

プリプロセッサで乗り切っています。

            if (DependencyService.Get<IDeviceVerifier>() is IDeviceVerifier verifier)
            {
#if USE_MOCK
                //nop
#else
                submission.DeviceVerificationPayload = await verifier?.VerifyAsync(submission);
#endif
            }

コードレベルで問題ないことは明らかですが、許されるでしょうか?(ユニットテストは、まだ分かりません)

b4e26e2
で以下のような結果が得られます。(settings.json で d0 か d1 かで、登録失敗と登録成功が試せます)
DiagSubmit

@keiji
Copy link
Collaborator

keiji commented May 16, 2021

プロダクトのコードに手を入れないという意味で、デバイス確認についてはIDeviceVerifierのモック(常に成功する)を作った方が良いと思います。

ちょうどIHttpDataServiceIStorageServiceでやっている例がありました。

#if USE_MOCK
container.Register<IHttpDataService, HttpDataServiceMock>(Reuse.Singleton);
container.Register<IStorageService, StorageServiceMock>(Reuse.Singleton);
#else
container.Register<IHttpDataService, HttpDataService>(Reuse.Singleton);
container.Register<IStorageService, StorageService>(Reuse.Singleton);
#endif

@i-maruyama
Copy link
Contributor Author

IDeviceVerifier は App.xaml.cs には登録されておらず、同じような枠組みでは無理そうでした。

IDeviceVerifierはデバイス依存で、以下のファイルを参照させる枠組みのようです(理解してません)。

  • cocoa/Covid19Radar/Covid19Radar.Android/Services/DeviceCheckService.cs
  • cocoa/Covid19Radar/Covid19Radar.iOS/Services/DeviceCheckService.cs

例えば、以下の部分に #if USE_MOCK で分岐させるようなコードがあれば、まだマシかもしれませんが、私にはわかりませんでした。

public interface IDeviceVerifier
{
Task<string> VerifyAsync(DiagnosisSubmissionParameter submission);
}

@keiji
Copy link
Collaborator

keiji commented May 17, 2021

ありがとうございます。たしかに現状のIDeviceVerifierはApp.xaml.csで取り扱われていませんね。

現状はDependencyServiceという古い仕組みで解決している様子です。

[assembly: Dependency(typeof(DeviceCheckService))]
namespace Covid19Radar.Droid.Services
{
public class DeviceCheckService : IDeviceVerifier

これはAndroidではバックグラウンドでの動作に支障を来たすことがあります(以前、Androidで一日一回再起動しなければならなかったのはこれが原因)。今はServiceLocatorという仕組みを使うようにしているので、ここも切り替えていきたいですね。

これに関しては別作業になりますからIssueを作りますね。

UPDATE
Issue(#185)とPull Request( #189 )作りました。

@i-maruyama
Copy link
Contributor Author

i-maruyama commented May 17, 2021

これはAndroidではバックグラウンドでの動作に支障を来たすことがあります
Issue(#185)とPull Request( #189 )作りました。

ありがとうございました。こちらは非常に安心になりました。
では、ExposureNotificationHandler.cs への変更は取り下げておいて、 PR しました。

実行確認手順としては、DebugPage PR #178 とDeviceCheckのMock PR #189 を取り込んで、 この Debug_Mock PR #191 を取り込んで、確認です。詳しくは #191 をご覧ください。

@i-maruyama
Copy link
Contributor Author

あと大規模な改変がある場合には DebugPage, Debug_Mock がバグを出す可能性もあるかもしれません。
そのときは、すっぱりファイル削除し、また新たに DebugPage, Debug_Mock を OSS 側に作り直してもらうという方針で進まれるのはいかがでしょうか?新規作成した今のうちに明文化しておくと、貢献者の不満も今後出ないと思います。

一度 DebugPage が消えたということなので、そんなことを思いました。

@keiji
Copy link
Collaborator

keiji commented May 17, 2022

デバッグ用の機能として、接触情報を登録することで、高リスク接触画面を表示できるようにしたので本Issueをクローズします。

@keiji keiji closed this as completed May 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
confirmed 開発内部管理用 enhancement 新しい機能や改善のリクエスト
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants