Bug #9897

"The Unsafe Browser can be used in all languages supported in Tails" scenario does not fail when switching locales fail

Added by intrigeri 2015-08-05 05:24:04 . Updated 2015-11-03 11:30:48 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Test suite
Target version:
Start date:
2015-08-05
Due date:
% Done:

100%

Feature Branch:
test/9897-unsafe-browser-langs
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Unsafe Browser
Deliverable for:

Description

I’ve seen that setting the fa_FA locale fails (which is not surprising, Farsi is not a region AFAIK, and there’s no such locale in Debian) during this test, but this doesn’t trigger a test failure, and the Unsafe Browser is started anyway (not sure with which locale). IMO we should make such a failure fatal, otherwise I suspect we’re actually not testing Unsafe Browser in the languages that we (fail to) set a locale for.


Files


Subtasks


History

#1 Updated by intrigeri 2015-08-05 05:25:05

  • blocks #8538 added

#2 Updated by anonym 2015-08-05 06:52:45

  • Assignee changed from kytv to intrigeri
  • QA Check set to Info Needed

intrigeri wrote:
> I’ve seen that setting the fa_FA locale fails

How does it fail? For me, fa_FA.utf8 results in the Farsi langpack being used.

> (which is not surprising, Farsi is not a region AFAIK, and there’s no such locale in Debian)

Indeed, the code is incorrect, and assumes the only locales we support are /usr/lib/locale/*.utf8, but for fa_IR there’s only /usr/lib/locale/fa_IR. Perhaps this code can be salvaged, otherwise we can extract them from the sources in config/chroot_local-includes/usr/share/tails/browser-localization/descriptions where we any way must keep an up-to-date list with what langpacks the Tor Browser installs. That would also have the benefit of picking more common locales, like de_DE instead of de_AT for German. Also, while it’s nice to extract stuff from the running system, I think it’s preferable to extract it from the sources when possible, since the sources are canon, and what end up in the built Tails image may be wrong. After all, we give no guarantees about the correctness of running the test suite from a different commit than what the image being tested was built from.

#3 Updated by intrigeri 2015-08-05 07:03:34

  • Assignee changed from intrigeri to kytv
  • QA Check changed from Info Needed to Dev Needed

> How does it fail?

In a Wheezy chroot with locales-all installed:

LC_ALL=fa_FA.utf-8
-bash: warning: setlocale: LC_ALL: cannot change locale (fa_FA.utf-8): No such file or directory

IIRC what I saw in the test suite was very similar.

> For me, fa_FA.utf8 results in the Farsi langpack being used.

Cool, but as I understand it, the point is to test the Unsafe Browser in a context that’s as close as possible to what users would see when they log in with Farsi language. This test is hackish already (which makes sense for test suite runtime optimization purposes), no need to add invalid locales on top of it :)

#4 Updated by intrigeri 2015-08-06 02:54:23

#5 Updated by intrigeri 2015-08-06 02:55:04

Same for vi_VI.utf-8 by the way.

#6 Updated by kytv 2015-09-04 20:43:55

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 20

#7 Updated by kytv 2015-09-04 20:58:35

  • Assignee changed from kytv to anonym
  • % Done changed from 20 to 50
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch set to kytv:test/9897-unsafe-browser-langs

anonym wrote:

> ….otherwise we can extract them from the sources in config/chroot_local-includes/usr/share/tails/browser-localization/descriptions where we any way must keep an up-to-date list with what langpacks the Tor Browser installs.

I wasn’t aware of this file when I originally did this test scenario. This makes the test much, much easier. ☺

Tested successfully both locally and on the isotester, multiple times.

#8 Updated by kytv 2015-09-05 04:13:12

I left this running multiple times during the night and I’m confident this branch does what it needs to. Whether there will be room for nitpicking remains to be seen but I’m happy with the changes here.

#9 Updated by anonym 2015-09-07 16:31:21

Applied in changeset commit:c986cbd5b613b710138afeab73346d34295b38ef.

#10 Updated by anonym 2015-09-07 16:34:14

  • Assignee changed from anonym to kytv
  • QA Check changed from Ready for QA to Info Needed

kytv wrote:
> anonym wrote:
>
> > ….otherwise we can extract them from the sources in config/chroot_local-includes/usr/share/tails/browser-localization/descriptions where we any way must keep an up-to-date list with what langpacks the Tor Browser installs.
>
> I wasn’t aware of this file when I originally did this test scenario. This makes the test much, much easier. ☺

To be fair, it was introduced after you wrote the initial test. :)

> I left this running multiple times during the night and I’m confident this branch does what it needs to. Whether there will be room for nitpicking remains to be seen but I’m happy with the changes here.

I was gonna ask for a clarification for why the scan regex works, and I actually saw that something must be wrong with it. Indeed it’s output is:

["ar_EG.utf8", "de_DE.utf8", "fa_IR.utf8", "fr_FR.utf8", "it_IT.utf8", "ko_KR.utf8", "nl_NL.utf8", "pl_PL.utf8", "ru_RU.utf8", "tr_TR.utf8", "vi_VN.utf8"]


so all the xx-YY:ZZ... description lines are skipped, like es-ES, and your version wouldn’t cope with three-character locales like ber if they ever were added. Also, you haven’t fixed what me and intrigeri spoke of in comments 2 and 3, which is the core of this ticket, i.e. that the some of the locales generated (like "fa_IR.utf8" and "vi_VN.utf8") are not valid, as can be see by the absence in /usr/lib/locale.

I pushed an improved version to the test/9897-unsafe-browser-langs which produces:

["ar_EG.utf8", "de_DE.utf8", "es_ES.utf8", "en_US.utf8", "fa_IR", "fr_FR.utf8", "it_IT.utf8", "ko_KR.utf8", "nl_NL.utf8", "pl_PL.utf8", "pt_PT.utf8", "ru_RU.utf8", "tr_TR.utf8", "vi_VN", "zh_CN.utf8"]


which seems like what we want. Sorry for essentially taking over this ticket. But I realized I was solving it as I was looking into the details. What do you think?

#11 Updated by kytv 2015-09-17 09:27:22

  • Assignee changed from kytv to anonym
  • QA Check changed from Info Needed to Dev Needed

anonym wrote:

> What do you think?

It’s absolutely the way to go. I didn’t realize that some of the locales weren’t UTF-8 capable hence going with my simpler method.

The now unused variable should be removed:

--- a/features/step_definitions/unsafe_browser.rb
+++ b/features/step_definitions/unsafe_browser.rb
@@ -5,7 +5,6 @@ When /^I see and accept the Unsafe Browser start verification$/ do
 end

 def supported_torbrowser_languages
-  langs = Array.new
   localization_descriptions = "#{Dir.pwd}/config/chroot_local-includes/usr/share/tails/browser-localization/descriptions"
   File.read(localization_descriptions).split("\n").map do |line|
     # The line will be of the form "xx:YY:..." or "xx-YY:YY:..."

After that’s done I think this could go to intri for nitpicking.

#12 Updated by anonym 2015-09-17 16:51:50

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

kytv wrote:
> The now unused variable should be removed:

Done.

> After that’s done I think this could go to intri for nitpicking.

Yeah, I guess that’s what we need since I took it over. Sorry, intri :/. I suppose this is another one for 1.7 but I leave that decision to you.

#13 Updated by intrigeri 2015-09-22 01:33:45

  • Target version changed from Tails_1.6 to Tails_1.7

#14 Updated by intrigeri 2015-09-22 02:17:01

  • Feature Branch changed from kytv:test/9897-unsafe-browser-langs to test/9897-unsafe-browser-langs

I assume that it’s not kytv’s branch I should now review.

#15 Updated by intrigeri 2015-09-22 02:20:22

  • Status changed from In Progress to Fix committed
  • % Done changed from 50 to 100

Applied in changeset commit:a516e929b551c2dcd9816bf4e8d4a42dbc52dbd9.

#16 Updated by intrigeri 2015-09-22 02:21:22

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

#17 Updated by anonym 2015-11-03 11:30:48

  • Status changed from Fix committed to Resolved