|
|
DescriptionVR: Support loading string resources in unit tests.
To this point, we haven't needed to test code that relies on translated
strings. The URL bar is about to require such a string, so we need to
include resource capabilities in the test target.
BUG=
Review-Url: https://codereview.chromium.org/2937263002
Cr-Commit-Position: refs/heads/master@{#480115}
Committed: https://chromium.googlesource.com/chromium/src/+/4d456ff29175aef972c8f24554ec1529afbfe2a7
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove test string used to validate change through the trybots. #
Total comments: 2
Messages
Total messages: 18 (9 generated)
The CQ bit was checked by cjgrant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cjgrant@chromium.org changed reviewers: + agrieve@chromium.org, sadrul@chromium.org
Andrew and Sadrul, this CL aims to let us use translated strings in our unit tests. I'm attempting to do as other unit test suites have done, but I might be breaking rules. For example, I'm reusing target otherwise only used by components. Overall, I don't have a good understanding of the .pak system, but I know that this gets our tests working. So any advice you have would be great! https://codereview.chromium.org/2937263002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2937263002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:254: auto chip_text = l10n_util::GetStringUTF16(IDS_DANGEROUS_VERBOSE_STATE); Folks, this is is temporary test code to exercise strings in the CQ dry run.
On 2017/06/15 19:03:55, cjgrant wrote: > Andrew and Sadrul, this CL aims to let us use translated strings in our unit > tests. > > I'm attempting to do as other unit test suites have done, but I might be > breaking rules. For example, I'm reusing target otherwise only used by > components. > > Overall, I don't have a good understanding of the .pak system, but I know that > this gets our tests working. So any advice you have would be great! > > https://codereview.chromium.org/2937263002/diff/1/chrome/browser/android/vr_s... > File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): > > https://codereview.chromium.org/2937263002/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/textures/url_bar_texture.cc:254: auto chip_text > = l10n_util::GetStringUTF16(IDS_DANGEROUS_VERBOSE_STATE); > Folks, this is is temporary test code to exercise strings in the CQ dry run. Might be helpful to know which strings you actually intend to use in tests. E.g. will you be added a gvr-specific .grd file?
On 2017/06/15 19:32:23, agrieve wrote: > On 2017/06/15 19:03:55, cjgrant wrote: > > Andrew and Sadrul, this CL aims to let us use translated strings in our unit > > tests. > > > > I'm attempting to do as other unit test suites have done, but I might be > > breaking rules. For example, I'm reusing target otherwise only used by > > components. > > > > Overall, I don't have a good understanding of the .pak system, but I know that > > this gets our tests working. So any advice you have would be great! > > > > > https://codereview.chromium.org/2937263002/diff/1/chrome/browser/android/vr_s... > > File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): > > > > > https://codereview.chromium.org/2937263002/diff/1/chrome/browser/android/vr_s... > > chrome/browser/android/vr_shell/textures/url_bar_texture.cc:254: auto > chip_text > > = l10n_util::GetStringUTF16(IDS_DANGEROUS_VERBOSE_STATE); > > Folks, this is is temporary test code to exercise strings in the CQ dry run. > > Might be helpful to know which strings you actually intend to use in tests. E.g. > will you be added a gvr-specific .grd file? There are no plans to add a VR-specific file. The few VR-specific strings we have are in chrome/app/generated_resources.grd (conditionally compiled in for VR), and we're tapping into existing strings in components/strings/grit as well. The chrome/app strings are here: See: https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=IDS...
On 2017/06/16 14:20:18, cjgrant wrote: > On 2017/06/15 19:32:23, agrieve wrote: > > On 2017/06/15 19:03:55, cjgrant wrote: > > > Andrew and Sadrul, this CL aims to let us use translated strings in our unit > > > tests. > > > > > > I'm attempting to do as other unit test suites have done, but I might be > > > breaking rules. For example, I'm reusing target otherwise only used by > > > components. > > > > > > Overall, I don't have a good understanding of the .pak system, but I know > that > > > this gets our tests working. So any advice you have would be great! > > > > > > > > > https://codereview.chromium.org/2937263002/diff/1/chrome/browser/android/vr_s... > > > File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): > > > > > > > > > https://codereview.chromium.org/2937263002/diff/1/chrome/browser/android/vr_s... > > > chrome/browser/android/vr_shell/textures/url_bar_texture.cc:254: auto > > chip_text > > > = l10n_util::GetStringUTF16(IDS_DANGEROUS_VERBOSE_STATE); > > > Folks, this is is temporary test code to exercise strings in the CQ dry run. > > > > Might be helpful to know which strings you actually intend to use in tests. > E.g. > > will you be added a gvr-specific .grd file? > > There are no plans to add a VR-specific file. The few VR-specific strings we > have are in chrome/app/generated_resources.grd (conditionally compiled in for > VR), and we're tapping into existing strings in components/strings/grit as well. > The chrome/app strings are here: > > See: > https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=IDS... I think this looks fine then. lgtm
The CQ bit was checked by cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2937263002/#ps20001 (title: "Remove test string used to validate change through the trybots.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2937263002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/BUILD.gn (right): https://codereview.chromium.org/2937263002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/BUILD.gn:228: if (is_android) { This code is in chrome/browser/android/. If this builds on non-android platforms, it should move elsewhere?
https://codereview.chromium.org/2937263002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/BUILD.gn (right): https://codereview.chromium.org/2937263002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/BUILD.gn:228: if (is_android) { On 2017/06/16 16:47:48, sadrul wrote: > This code is in chrome/browser/android/. If this builds on non-android > platforms, it should move elsewhere? Indeed it should. We're soon to migrate this out to chrome/browser/vr (you actually suggested this location :). The first steps were splitting out the common library and getting the tests working across platforms, but we didn't move the code because it would have created branch merge nightmares. I expect we'll move the code next week, now that M-60 merges are done. Thanks for noticing this though, it means we have the right intentions.
cool. lgtm
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497628859870280, "parent_rev": "1a688b5cdec13a4e11895f665240c2a2f1f95537", "commit_rev": "4d456ff29175aef972c8f24554ec1529afbfe2a7"}
Message was sent while issue was closed.
Description was changed from ========== VR: Support loading string resources in unit tests. To this point, we haven't needed to test code that relies on translated strings. The URL bar is about to require such a string, so we need to include resource capabilities in the test target. BUG= ========== to ========== VR: Support loading string resources in unit tests. To this point, we haven't needed to test code that relies on translated strings. The URL bar is about to require such a string, so we need to include resource capabilities in the test target. BUG= Review-Url: https://codereview.chromium.org/2937263002 Cr-Commit-Position: refs/heads/master@{#480115} Committed: https://chromium.googlesource.com/chromium/src/+/4d456ff29175aef972c8f24554ec... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4d456ff29175aef972c8f24554ec... |