Feature #8918
Determine supported Unsafe Browser languages programmatically
100%
Description
In Feature #8701 I implemented a test in which the locales were hardcoded. It would be better to do this based upon the languages installed for the Tor Browser.
Subtasks
History
#1 Updated by kytv 2015-02-18 23:02:52
- Assignee changed from kytv to anonym
- QA Check set to Ready for QA
- Feature Branch set to kytv:test/8918-unsafe-browser-languages-8701-take-2
#2 Updated by BitingBird 2015-02-23 06:03:47
- Status changed from New to In Progress
#3 Updated by kytv 2015-02-25 18:54:53
Patch re-applied to the rewritten git repo.
#4 Updated by anonym 2015-03-20 10:46:19
- Assignee changed from anonym to kytv
- QA Check changed from Ready for QA to Dev Needed
The test runs well, at least when there’s no error. Some comments and suggestions:
> + supported_langs = exts.scan(/langpack-([a-zA-Z_-]+).*/)
Please:
- add
.flatten
, then you can simplifylang[0]
tolang
in the loop body.
- add
.map { |x| x.gsub('-','_') }
so thegsub
thing can be removed from the loop body. This can be done in a separate call/line, with a comment explaining why we need it (i.e. because we’re gonna compare it with something using_
instead of-
.
> nomatch = proc { “#{lang}_#{lang.upcase}.utf8” }
> langs << locales.find(nomatch) { |l| l.match(/^#{lang}/) }
Please add a comment explaining that find(nomatch)
will return an “approximation” of the expected locale if it cannot be found among the ones installed in Tails. Or however you prefer to phrase it.
Yeah, I had a hard time understanding the loop body, hence these suggestions.
> +Then /^the Unsafe Browser works in all supported languages$/ do
An error for a particular locale will be really opaque in this step. We’ll just get a generic FindFailed
error for UnsafeBrowserStartVerification.png
with no indication which locale that fialed. I suggest rescue
:ing FindFailed
and raise
:ing an improved error message mentioning the locale.
Or even better: since this is a compact version of multiple scenarios, I think we shouldn’t throw the error immediately. Instead, we always run the full loop, record which locales we had a FindFailed
for, and if there was some we throw a single error reporting all failed locales after the loop.
> + langs = supported_torbrowser_languages
> + langs.each do |lang|
IMHO supported_torbrowser_languages.each do |lang|
is more readable.
#5 Updated by kytv 2015-03-21 22:14:01
anonym wrote:
> The test runs well, at least when there’s no error.
Woot.
> Some comments and suggestions:
>
> > + supported_langs = exts.scan(/langpack-([a-zA-Z_-]+).*/)
>
> Please:
>
> * add .flatten
, then you can simplify lang[0]
to lang
in the loop body.
>
> * add .map { |x| x.gsub('-','_') }
so the gsub
thing can be removed from the loop body. This can be done in a separate call/line, with a comment explaining why we need it (i.e. because we’re gonna compare it with something using _
instead of -
.
Nice, I wasn’t aware of .map
or .flatten
.
>
> > nomatch = proc { “#{lang}_#{lang.upcase}.utf8” }
> > langs << locales.find(nomatch) { |l| l.match(/^#{lang}/) }
>
> Please add a comment explaining that find(nomatch)
will return an “approximation” of the expected locale if it cannot be found among the ones installed in Tails. Or however you prefer to phrase it.
>
> Yeah, I had a hard time understanding the loop body, hence these suggestions.
OK.
>
> > +Then /^the Unsafe Browser works in all supported languages$/ do
>
> An error for a particular locale will be really opaque in this step. We’ll just get a generic FindFailed
error for UnsafeBrowserStartVerification.png
with no indication which locale that fialed. I suggest rescue
:ing FindFailed
and raise
:ing an improved error message mentioning the locale.
>
> Or even better: since this is a compact version of multiple scenarios, I think we shouldn’t throw the error immediately. Instead, we always run the full loop, record which locales we had a FindFailed
for, and if there was some we throw a single error reporting all failed locales after the loop.
This is actually what I wanted to do but hadn’t figured out how to make it work. Perhaps I’ve learned enough since I started this branch to make it happen…
>
> > + langs = supported_torbrowser_languages
> > + langs.each do |lang|
>
> IMHO supported_torbrowser_languages.each do |lang|
is more readable.
I agree. In retrospect, I don’t know why I thought it’d be better to have a shorter var name there.
#6 Updated by kytv 2015-03-22 00:52:08
- % Done changed from 30 to 40
anonym wrote:
> * add .map { |x| x.gsub('-','_') }
so the gsub
thing can be removed from the loop body. This can be done in a separate call/line, with a comment explaining why we need it (i.e. because we’re gonna compare it with something using _
instead of -
.
Actually, I found that my test was buggy, this gsub
bit can be removed, and that I should just get the language code.
The problem that these tests are trying to catch was the case of guess_best_tor_browser_locale
doing the wrong thing, such as with pt_BR
or es_AR
. pt_PT
and es_ES
worked fine. So instead of this I’ll do something like
- supported_langs = exts.scan(/langpack-([a-zA-Z_-]+).*/).flatten
+ supported_langs = exts.scan(/langpack-([a-z]+).*/).flatten
This finds the problem in 1.2.2. My original version of this branch did not.
I wrote:
> This is actually what I wanted to do but hadn’t figured out how to make it work. Perhaps I’ve learned enough since I started this branch to make it happen…
..and it seems I have. What I’ve implemented (but not pushed) seems to be working. With the 1.2.2 ISO:
Scenario: The Unsafe Browser can be used in all languges supported in Tails # features/unsafe_browser.feature:29
Then the Unsafe Browser works in all supported languages # features/step_definitions/unsafe_browser.rb:37
Test failed in ["es_AR.utf8", "pt_BR.utf8"].
<false> is not true. (Test::Unit::AssertionFailedError)
./features/step_definitions/unsafe_browser.rb:53:in `/^the Unsafe Browser works in all supported languages$/'
features/unsafe_browser.feature:30:in `Then the Unsafe Browser works in all supported languages'
#7 Updated by kytv 2015-03-22 20:11:09
- Assignee changed from kytv to anonym
- % Done changed from 40 to 50
- QA Check changed from Dev Needed to Ready for QA
Everything pointed out has been addressed. The tests pass with 1.3.1 (as they should) and fail with 1.2.2 (as they should).
#8 Updated by anonym 2015-03-25 09:44:24
- Status changed from In Progress to Fix committed
- % Done changed from 50 to 100
Applied in changeset commit:f08617c74d89486fbf03e483bed7d616580ea487.
#9 Updated by anonym 2015-03-25 09:44:50
- Assignee deleted (
anonym) - QA Check changed from Ready for QA to Pass
#10 Updated by anonym 2015-03-31 19:02:17
- Status changed from Fix committed to Resolved