Project

General

Profile

Feature #9339

Feature #8543: Write regression tests and tests for new features

Test that Seahorse is configured to use the correct keyserver

Added by kytv over 4 years ago. Updated over 4 years ago.

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

100%

Feature Branch:
kytv:test/9339-seahorse-keyserver
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Related issues

Related to Tails - Bug #9233: Seahorse's configured keyservers are not the same as those in gpg.conf Resolved 04/14/2015

Associated revisions

Revision a1d5afd4 (diff)
Added by kytv over 4 years ago

Test that Seahorse is configured to use the correct keyserver

Will-fix: #9339

Revision 75a4e7b8
Added by anonym over 4 years ago

Merge remote-tracking branch 'kytv/test/9339-seahorse-keyserver' into stable

Fix-committed: #9339

History

#1 Updated by kytv over 4 years ago

  • Related to Bug #9233: Seahorse's configured keyservers are not the same as those in gpg.conf added

#2 Updated by kytv over 4 years ago

  • Assignee changed from kytv to anonym
  • QA Check set to Ready for QA
  • Feature Branch set to kytv:test/9339-seahorse-keyserver

#4 Updated by anonym over 4 years ago

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

IMHO we should refrain from adding stuff into checks.feature (see #5707, and it will truly become simple to split it as much as we want when #6094 is finished) when there are places with at least some relation to what's being tested. Isn't torified_gnupg.feature more suitable for Scenario: Seahorse is configured to use the correct keyserver? If so, please move the step definition to torified_gnupg.rb as well.

+ assert_equal(1, gnome_keyserver.count, 'Seahorse only have one keyserver configured.')

The message is confusing. If I got that error I would think that it's a problem that only one key server is configured. I think there's a missing "must" or "should" in front of "only".

+  gnome_keyserver = @vm.execute_successfully('gsettings get org.gnome.crypto.pgp keyservers',
+                                             LIVE_USER).stdout.chomp.gsub(/[\s'"\[\]]/,'').split(',')

The .stdout.chomp.gsub(...).split can be simplified since the output we get is yaml (or at least is compatible with it), so we can get the array simply with

@gnome_keyservers = YAML.load(@vm.execute_successfully('gsettings get org.gnome.crypto.pgp keyservers', LIVE_USER).stdout)

instead. Let's avoid doing ad-hoc parsing when we can! Also note that I changed the variable name to the plural version since we could get more than one (in the failure case).

Other than this nitpicking, this scenario does the job.

#5 Updated by kytv over 4 years ago

anonym wrote:

Isn't torified_gnupg.feature more suitable for Scenario: Seahorse is configured to use the correct keyserver? If so, please move the step definition to torified_gnupg.rb as well.

I only tucked it into checks.feature because most of that feature runs without needing access to the network, so I thought this would make the test go a wee bit faster, but I can move it when I make the other changes requested.

#6 Updated by kytv over 4 years ago

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

anonym wrote:

+ assert_equal(1, gnome_keyserver.count, 'Seahorse only have one keyserver configured.')

The message is confusing. If I got that error I would think that it's a problem that only one key server is configured. I think there's a missing "must" or "should" in front of "only".

That's what was intended. How I missed that I dunno. :|

[...]

The .stdout.chomp.gsub(...).split can be simplified since the output we get is yaml (or at least is compatible with it), so we can get the array simply with

Thanks for that pointer, I didn't realize YAML.load would work.

I force pushed a fixed version, mostly force-pushing because I thought moving everything to torified_gnupg.* might be messier than if I just redid it. I can restore the original branch and make the modifications there if it'd be preferred.

#7 Updated by kytv over 4 years ago

#8 Updated by anonym over 4 years ago

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

#9 Updated by anonym over 4 years ago

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

It looks great now. Merged!

However, I have a suggestion:

+  assert_equal(CONFIGURED_KEYSERVER_HOSTNAME.gsub('hkps.', 'hkp://'), @gnome_keyservers[0])

Just some general notes about gsub() and sub(), which you are fond of using (which is completely fine, btw!):

  1. We don't need gsub() here and should use sub() since we only expect one replacement. More than one would cause issues.
  2. Both sub() and gsub() can take a regex as the replacement pattern, and I think it's good practice to anchor the patterns to where we expect them when possible, e.g. @.sub(/^hkps\./, 'hkp://') in this case.

I won't block on this here since I cannot come up with a situation where this would go unnoticed and we would get a false positive, but please take it into consideration in the future.

#10 Updated by kytv over 4 years ago

anonym wrote:

I won't block on this here since I cannot come up with a situation where this would go unnoticed and we would get a false positive, but please take it into consideration in the future.

Noted, thanks for the advice/educating me. :)

#11 Updated by intrigeri over 4 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF