[Missing Tests] : ExtensionSettings description too long |
|||||
Issue descriptionAutomated tests for the below commit have been missing.Would it be possible to add test coverage to avoid regressions in future? CL: === https://chromium.googlesource.com/chromium/src.git/+/5710f8d7a584898c5e9d2caf5dc00f71d268e790 Ref Bug: ======== https://bugs.chromium.org/p/chromium/issues/detail?id=789087
,
Dec 15 2017
So if the issue was in exceeding some fixed limit on the number of characters in the policy description, should we just add a check into syntax_check_policy_template_json.py to enforce this constraint at the presubmit stage?
,
Dec 18 2017
According to this random site on the internet: https://books.google.de/books?id=uJ5CAwAAQBAJ&pg=PT518&lpg=PT518&dq=%22adm%22+template+explain+text+character+limit&source=bl&ots=uwYS0ujG8d&sig=tDjYdR-IoJ6Ym_aY8NsdJimuvJo&hl=en&sa=X&ved=0ahUKEwi0isnNoJPYAhUMVxQKHdR8D-wQ6AEIKjAA#v=onepage&q=%22adm%22%20template%20explain%20text%20character%20limit&f=false the character limit for the description (aka EXPLAIN text in ADM templates) is 4096 characters. ADMX doesn't seem to have this restriction. One option would be to just kill ADM since ADMX is supported since 2008, but Julian mentioned that ADM is still used. Adding a check to syntax_check_policy_template_json.py might not catch everything since that check operates on the English text with macros only, not on the final translated text with macros replaced. You'd have to run it through the translation tool first to be sure. A workaround could be to add a safety margin (e.g. check >= 3072 characters).
,
Dec 18 2017
To add on that, I think we should add this check since it's very simple and likely to catch issues.
,
Dec 22 2017
Agreed. It's only worrying that this "4096 characters" is too vaguely defined, thinking about Unicode complications: is this talking about Unicode code points or UTF-16 code units or UTF-16 encoding bytes? I'm guessing, though, that it's likely to be UTF-16 code units, which would mean that practically this should be of the same order as the code point count in the original string (which is easy to obtain in Python).
,
Dec 22 2017
Good point. I agree it's vague. We'd have to try that out. If it's UTF-16 bytes (i.e. raw memory size), then we'd have to cap at 1536 characters. The description in this CL had >6500 ASCII characters (measured the 'dumb' way), so I'm guessing they mean code points.
,
Dec 24
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 27
CL ready for review here https://chromium-review.googlesource.com/c/chromium/src/+/1391509
,
Jan 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fe450a933106c0e4c51022bb20118147a25fa4bb commit fe450a933106c0e4c51022bb20118147a25fa4bb Author: Julian Pastarmov <pastarmovj@chromium.org> Date: Mon Jan 07 13:54:07 2019 Verify the maximal description length for policies The ADM policy templates have a limit of 4k characters for each string. The CL adds a presubmit check with a hard error at 4k characters and a warning at about 3k because any translation can easily exceed the English text. It would be nice to be able to verify that no translation exceeds the limit too but since translations are only started on release branches the point at which this issue will be discovered will be a rather inconvenient point in time to fix it. Also given that the issue is only present with the old ADM format it doesn't deserve much more than what is done already to truncate bad translations to 4k chars and here to try to avoid the need for that. BUG= 792864 TEST=presubmit passes Change-Id: Ib1db65f416c1ac9273f0825d6a604120d0db9de8 Reviewed-on: https://chromium-review.googlesource.com/c/1391509 Commit-Queue: Julian Pastarmov <pastarmovj@chromium.org> Reviewed-by: Lutz Justen <ljusten@chromium.org> Cr-Commit-Position: refs/heads/master@{#620312} [modify] https://crrev.com/fe450a933106c0e4c51022bb20118147a25fa4bb/components/policy/tools/syntax_check_policy_template_json.py
,
Jan 7
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by pastarmovj@chromium.org
, Dec 7 2017Labels: -Pri-1 Pri-2
Owner: ----
Status: Available (was: Assigned)