Tails

#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

#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 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 -.

> 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.

#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