[ARVADOS] updated: 1.3.0-3131-gf4d0ea9ca
Git user
git at public.arvados.org
Mon Sep 14 20:39:35 UTC 2020
Summary of changes:
services/api/lib/config_loader.rb | 16 ++---
services/api/lib/enable_jobs_api.rb | 23 ++++---
.../arvados/v1/groups_controller_test.rb | 6 +-
.../arvados/v1/schema_controller_test.rb | 4 +-
.../functional/user_sessions_controller_test.rb | 2 +-
services/api/test/test_helper.rb | 18 +++++-
services/api/test/unit/collection_test.rb | 8 +--
services/api/test/unit/container_request_test.rb | 20 ++----
services/api/test/unit/job_test.rb | 2 +-
services/api/test/unit/log_test.rb | 6 +-
services/api/test/unit/user_test.rb | 72 +++++++++++-----------
11 files changed, 91 insertions(+), 86 deletions(-)
via f4d0ea9ca3ef4badd38957f794d12474831cee44 (commit)
via e696a09cd482ddedeec742127297e90287012356 (commit)
via c986fef1ed1ad3967639f75d6e2d6ab0c63e4937 (commit)
via 6350ddac78dbd2b29e5dc4117fd57300f7f8a098 (commit)
from c8a640d527db530875b52c9975a34709a2f32f7c (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit f4d0ea9ca3ef4badd38957f794d12474831cee44
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Mon Sep 14 17:36:43 2020 -0300
16826: Moves configuration set-up to TestCase's setup() method.
Having a failing assertion on the teardown() method before the config
restoration code has the side effect of failing all the tests following
the one that made an illegal config change. This updates fixes that.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 12e642d0e..5dc77cb98 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -62,7 +62,7 @@ class ActiveSupport::TestCase
include ArvadosTestSupport
include CurrentApiClient
- teardown do
+ setup do
Thread.current[:api_client_ip_address] = nil
Thread.current[:api_client_authorization] = nil
Thread.current[:api_client_uuid] = nil
@@ -72,6 +72,14 @@ class ActiveSupport::TestCase
restore_configuration
end
+ teardown do
+ # Confirm that any changed configuration doesn't include non-symbol keys
+ $arvados_config.keys.each do |conf_name|
+ conf = Rails.configuration.send(conf_name)
+ confirm_keys_as_symbols(conf, conf_name) if conf.respond_to?('keys')
+ end
+ end
+
def assert_equal(expect, *args)
if expect.nil?
assert_nil(*args)
@@ -108,11 +116,6 @@ class ActiveSupport::TestCase
end
def restore_configuration
- # Confirm that any changed configuration doesn't include non-symbol keys
- $arvados_config.keys.each do |conf_name|
- conf = Rails.configuration.send("#{conf_name}")
- confirm_keys_as_symbols(conf, conf_name) if conf.respond_to?('keys')
- end
# Restore configuration settings changed during tests
ConfigLoader.copy_into_config $arvados_config, Rails.configuration
ConfigLoader.copy_into_config $remaining_config, Rails.configuration
commit e696a09cd482ddedeec742127297e90287012356
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Mon Sep 14 17:23:09 2020 -0300
16826: Fixes config maps assignments, mostly on tests.
The new config check brought to the spotlight new tests that are setting
configurations incorrectly.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/lib/enable_jobs_api.rb b/services/api/lib/enable_jobs_api.rb
index 1a96a81ad..cef76f08a 100644
--- a/services/api/lib/enable_jobs_api.rb
+++ b/services/api/lib/enable_jobs_api.rb
@@ -2,16 +2,19 @@
#
# SPDX-License-Identifier: AGPL-3.0
-Disable_update_jobs_api_method_list = {"jobs.create"=>{},
- "pipeline_instances.create"=>{},
- "pipeline_templates.create"=>{},
- "jobs.update"=>{},
- "pipeline_instances.update"=>{},
- "pipeline_templates.update"=>{},
- "job_tasks.create"=>{},
- "job_tasks.update"=>{}}
+Disable_update_jobs_api_method_list = ConfigLoader.to_OrderedOptions({
+ "jobs.create"=>{},
+ "pipeline_instances.create"=>{},
+ "pipeline_templates.create"=>{},
+ "jobs.update"=>{},
+ "pipeline_instances.update"=>{},
+ "pipeline_templates.update"=>{},
+ "job_tasks.create"=>{},
+ "job_tasks.update"=>{}
+ })
-Disable_jobs_api_method_list = {"jobs.create"=>{},
+Disable_jobs_api_method_list = ConfigLoader.to_OrderedOptions({
+ "jobs.create"=>{},
"pipeline_instances.create"=>{},
"pipeline_templates.create"=>{},
"jobs.get"=>{},
@@ -36,7 +39,7 @@ Disable_jobs_api_method_list = {"jobs.create"=>{},
"jobs.show"=>{},
"pipeline_instances.show"=>{},
"pipeline_templates.show"=>{},
- "job_tasks.show"=>{}}
+ "job_tasks.show"=>{}})
def check_enable_legacy_jobs_api
# Create/update is permanently disabled (legacy functionality has been removed)
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index f038b1e55..f413188b5 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -430,10 +430,8 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
end
test 'get contents with jobs and pipeline instances disabled' do
- conf = ActiveSupport::OrderedOptions.new
- conf['jobs.index'] = {}
- conf['pipeline_instances.index'] = {}
- Rails.configuration.API.DisabledAPIs = conf
+ Rails.configuration.API.DisabledAPIs = ConfigLoader.to_OrderedOptions(
+ {'jobs.index'=>{}, 'pipeline_instances.index'=>{}})
authorize_with :active
get :contents, params: {
diff --git a/services/api/test/functional/arvados/v1/schema_controller_test.rb b/services/api/test/functional/arvados/v1/schema_controller_test.rb
index 3dd343b13..764f3a8d1 100644
--- a/services/api/test/functional/arvados/v1/schema_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/schema_controller_test.rb
@@ -65,8 +65,8 @@ class Arvados::V1::SchemaControllerTest < ActionController::TestCase
end
test "non-empty disable_api_methods" do
- Rails.configuration.API.DisabledAPIs =
- {'jobs.create'=>{}, 'pipeline_instances.create'=>{}, 'pipeline_templates.create'=>{}}
+ Rails.configuration.API.DisabledAPIs = ConfigLoader.to_OrderedOptions(
+ {'jobs.create'=>{}, 'pipeline_instances.create'=>{}, 'pipeline_templates.create'=>{}})
get :index
assert_response :success
discovery_doc = JSON.parse(@response.body)
diff --git a/services/api/test/functional/user_sessions_controller_test.rb b/services/api/test/functional/user_sessions_controller_test.rb
index cd475dea4..d979208d3 100644
--- a/services/api/test/functional/user_sessions_controller_test.rb
+++ b/services/api/test/functional/user_sessions_controller_test.rb
@@ -68,7 +68,7 @@ class UserSessionsControllerTest < ActionController::TestCase
test "login to LoginCluster" do
Rails.configuration.Login.LoginCluster = 'zbbbb'
- Rails.configuration.RemoteClusters['zbbbb'] = {'Host' => 'zbbbb.example.com'}
+ Rails.configuration.RemoteClusters['zbbbb'] = ConfigLoader.to_OrderedOptions({'Host' => 'zbbbb.example.com'})
api_client_page = 'http://client.example.com/home'
get :login, params: {return_to: api_client_page}
assert_response :redirect
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index addea8306..48cae5afe 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -1044,10 +1044,10 @@ class CollectionTest < ActiveSupport::TestCase
end
test "create collections with managed properties" do
- Rails.configuration.Collections.ManagedProperties = {
+ Rails.configuration.Collections.ManagedProperties = ConfigLoader.to_OrderedOptions({
'default_prop1' => {'Value' => 'prop1_value'},
'responsible_person_uuid' => {'Function' => 'original_owner'}
- }
+ })
# Test collection without initial properties
act_as_user users(:active) do
c = create_collection 'foo', Encoding::US_ASCII
@@ -1076,9 +1076,9 @@ class CollectionTest < ActiveSupport::TestCase
end
test "update collection with protected managed properties" do
- Rails.configuration.Collections.ManagedProperties = {
+ Rails.configuration.Collections.ManagedProperties = ConfigLoader.to_OrderedOptions({
'default_prop1' => {'Value' => 'prop1_value', 'Protected' => true},
- }
+ })
act_as_user users(:active) do
c = create_collection 'foo', Encoding::US_ASCII
assert c.valid?
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 58216c2b6..90de800b2 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -576,9 +576,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
test "Container.resolve_container_image(pdh)" do
set_user_from_auth :active
[[:docker_image, 'v1'], [:docker_image_1_12, 'v2']].each do |coll, ver|
- conf = ActiveSupport::OrderedOptions.new
- conf[ver] = {}
- Rails.configuration.Containers.SupportedDockerImageFormats = conf
+ Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({ver=>{}})
pdh = collections(coll).portable_data_hash
resolved = Container.resolve_container_image(pdh)
assert_equal resolved, pdh
@@ -604,9 +602,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
end
test "migrated docker image" do
- conf = ActiveSupport::OrderedOptions.new
- conf['v2'] = {}
- Rails.configuration.Containers.SupportedDockerImageFormats = conf
+ Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({'v2'=>{}})
add_docker19_migration_link
# Test that it returns only v2 images even though request is for v1 image.
@@ -624,9 +620,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
end
test "use unmigrated docker image" do
- conf = ActiveSupport::OrderedOptions.new
- conf['v1'] = {}
- Rails.configuration.Containers.SupportedDockerImageFormats = conf
+ Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({'v1'=>{}})
add_docker19_migration_link
# Test that it returns only supported v1 images even though there is a
@@ -645,9 +639,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
end
test "incompatible docker image v1" do
- conf = ActiveSupport::OrderedOptions.new
- conf['v1'] = {}
- Rails.configuration.Containers.SupportedDockerImageFormats = conf
+ Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({'v1'=>{}})
add_docker19_migration_link
# Don't return unsupported v2 image even if we ask for it directly.
@@ -660,9 +652,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
end
test "incompatible docker image v2" do
- conf = ActiveSupport::OrderedOptions.new
- conf['v2'] = {}
- Rails.configuration.Containers.SupportedDockerImageFormats = conf
+ Rails.configuration.Containers.SupportedDockerImageFormats = ConfigLoader.to_OrderedOptions({'v2'=>{}})
# No migration link, don't return unsupported v1 image,
set_user_from_auth :active
diff --git a/services/api/test/unit/job_test.rb b/services/api/test/unit/job_test.rb
index 0e8cc4853..c529aab8b 100644
--- a/services/api/test/unit/job_test.rb
+++ b/services/api/test/unit/job_test.rb
@@ -117,7 +117,7 @@ class JobTest < ActiveSupport::TestCase
'locator' => BAD_COLLECTION,
}.each_pair do |spec_type, image_spec|
test "Job validation fails with nonexistent Docker image #{spec_type}" do
- Rails.configuration.RemoteClusters = {}
+ Rails.configuration.RemoteClusters = ConfigLoader.to_OrderedOptions({})
job = Job.new job_attrs(runtime_constraints:
{'docker_image' => image_spec})
assert(job.invalid?, "nonexistent Docker image #{spec_type} #{image_spec} was valid")
diff --git a/services/api/test/unit/log_test.rb b/services/api/test/unit/log_test.rb
index 59fe3624d..58f0cddb7 100644
--- a/services/api/test/unit/log_test.rb
+++ b/services/api/test/unit/log_test.rb
@@ -282,9 +282,7 @@ class LogTest < ActiveSupport::TestCase
end
test "non-empty configuration.unlogged_attributes" do
- conf = ActiveSupport::OrderedOptions.new
- conf["manifest_text"] = {}
- Rails.configuration.AuditLogs.UnloggedAttributes = conf
+ Rails.configuration.AuditLogs.UnloggedAttributes = ConfigLoader.to_OrderedOptions({"manifest_text"=>{}})
txt = ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:foo\n"
act_as_system_user do
@@ -299,7 +297,7 @@ class LogTest < ActiveSupport::TestCase
end
test "empty configuration.unlogged_attributes" do
- Rails.configuration.AuditLogs.UnloggedAttributes = ActiveSupport::OrderedOptions.new
+ Rails.configuration.AuditLogs.UnloggedAttributes = ConfigLoader.to_OrderedOptions({})
txt = ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:foo\n"
act_as_system_user do
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 9e4a99c59..b6d66230d 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -110,9 +110,7 @@ class UserTest < ActiveSupport::TestCase
end
test "new username set avoiding blacklist" do
- conf = ActiveSupport::OrderedOptions.new
- conf["root"] = {}
- Rails.configuration.Users.AutoSetupUsernameBlacklist = conf
+ Rails.configuration.Users.AutoSetupUsernameBlacklist = ConfigLoader.to_OrderedOptions({"root"=>{}})
check_new_username_setting("root", "root2")
end
@@ -342,48 +340,52 @@ class UserTest < ActiveSupport::TestCase
assert_equal(user.first_name, 'first_name_for_newly_created_user_updated')
end
+ active_notify_list = ConfigLoader.to_OrderedOptions({"active-notify at example.com"=>{}})
+ inactive_notify_list = ConfigLoader.to_OrderedOptions({"inactive-notify at example.com"=>{}})
+ empty_notify_list = ConfigLoader.to_OrderedOptions({})
+
test "create new user with notifications" do
set_user_from_auth :admin
- create_user_and_verify_setup_and_notifications true, {'active-notify-address at example.com'=>{}}, {'inactive-notify-address at example.com'=>{}}, nil, nil
- create_user_and_verify_setup_and_notifications true, {'active-notify-address at example.com'=>{}}, {}, nil, nil
- create_user_and_verify_setup_and_notifications true, {}, [], nil, nil
- create_user_and_verify_setup_and_notifications false, {'active-notify-address at example.com'=>{}}, {'inactive-notify-address at example.com'=>{}}, nil, nil
- create_user_and_verify_setup_and_notifications false, {}, {'inactive-notify-address at example.com'=>{}}, nil, nil
- create_user_and_verify_setup_and_notifications false, {}, {}, nil, nil
+ create_user_and_verify_setup_and_notifications true, active_notify_list, inactive_notify_list, nil, nil
+ create_user_and_verify_setup_and_notifications true, active_notify_list, empty_notify_list, nil, nil
+ create_user_and_verify_setup_and_notifications true, empty_notify_list, empty_notify_list, nil, nil
+ create_user_and_verify_setup_and_notifications false, active_notify_list, inactive_notify_list, nil, nil
+ create_user_and_verify_setup_and_notifications false, empty_notify_list, inactive_notify_list, nil, nil
+ create_user_and_verify_setup_and_notifications false, empty_notify_list, empty_notify_list, nil, nil
end
[
# Easy inactive user tests.
- [false, {}, {}, "inactive-none at example.com", false, false, "inactivenone"],
- [false, {}, {}, "inactive-vm at example.com", true, false, "inactivevm"],
- [false, {}, {}, "inactive-repo at example.com", false, true, "inactiverepo"],
- [false, {}, {}, "inactive-both at example.com", true, true, "inactiveboth"],
+ [false, empty_notify_list, empty_notify_list, "inactive-none at example.com", false, false, "inactivenone"],
+ [false, empty_notify_list, empty_notify_list, "inactive-vm at example.com", true, false, "inactivevm"],
+ [false, empty_notify_list, empty_notify_list, "inactive-repo at example.com", false, true, "inactiverepo"],
+ [false, empty_notify_list, empty_notify_list, "inactive-both at example.com", true, true, "inactiveboth"],
# Easy active user tests.
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "active-none at example.com", false, false, "activenone"],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "active-vm at example.com", true, false, "activevm"],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "active-repo at example.com", false, true, "activerepo"],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "active-both at example.com", true, true, "activeboth"],
+ [true, active_notify_list, inactive_notify_list, "active-none at example.com", false, false, "activenone"],
+ [true, active_notify_list, inactive_notify_list, "active-vm at example.com", true, false, "activevm"],
+ [true, active_notify_list, inactive_notify_list, "active-repo at example.com", false, true, "activerepo"],
+ [true, active_notify_list, inactive_notify_list, "active-both at example.com", true, true, "activeboth"],
# Test users with malformed e-mail addresses.
- [false, {}, {}, nil, true, true, nil],
- [false, {}, {}, "arvados", true, true, nil],
- [false, {}, {}, "@example.com", true, true, nil],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "*!*@example.com", true, false, nil],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "*!*@example.com", false, false, nil],
+ [false, empty_notify_list, empty_notify_list, nil, true, true, nil],
+ [false, empty_notify_list, empty_notify_list, "arvados", true, true, nil],
+ [false, empty_notify_list, empty_notify_list, "@example.com", true, true, nil],
+ [true, active_notify_list, inactive_notify_list, "*!*@example.com", true, false, nil],
+ [true, active_notify_list, inactive_notify_list, "*!*@example.com", false, false, nil],
# Test users with various username transformations.
- [false, {}, {}, "arvados at example.com", false, false, "arvados2"],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "arvados at example.com", false, false, "arvados2"],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "root at example.com", true, false, "root2"],
- [false, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "root at example.com", true, false, "root2"],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "roo_t at example.com", false, true, "root2"],
- [false, {}, {}, "^^incorrect_format at example.com", true, true, "incorrectformat"],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "&4a_d9. at example.com", true, true, "ad9"],
- [true, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "&4a_d9. at example.com", false, false, "ad9"],
- [false, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "&4a_d9. at example.com", true, true, "ad9"],
- [false, {"active-notify at example.com"=>{}}, {"inactive-notify at example.com"=>{}}, "&4a_d9. at example.com", false, false, "ad9"],
+ [false, empty_notify_list, empty_notify_list, "arvados at example.com", false, false, "arvados2"],
+ [true, active_notify_list, inactive_notify_list, "arvados at example.com", false, false, "arvados2"],
+ [true, active_notify_list, inactive_notify_list, "root at example.com", true, false, "root2"],
+ [false, active_notify_list, inactive_notify_list, "root at example.com", true, false, "root2"],
+ [true, active_notify_list, inactive_notify_list, "roo_t at example.com", false, true, "root2"],
+ [false, empty_notify_list, empty_notify_list, "^^incorrect_format at example.com", true, true, "incorrectformat"],
+ [true, active_notify_list, inactive_notify_list, "&4a_d9. at example.com", true, true, "ad9"],
+ [true, active_notify_list, inactive_notify_list, "&4a_d9. at example.com", false, false, "ad9"],
+ [false, active_notify_list, inactive_notify_list, "&4a_d9. at example.com", true, true, "ad9"],
+ [false, active_notify_list, inactive_notify_list, "&4a_d9. at example.com", false, false, "ad9"],
].each do |active, new_user_recipients, inactive_recipients, email, auto_setup_vm, auto_setup_repo, expect_username|
test "create new user with auto setup #{active} #{email} #{auto_setup_vm} #{auto_setup_repo}" do
set_user_from_auth :admin
@@ -571,7 +573,6 @@ class UserTest < ActiveSupport::TestCase
assert_not_nil resp_user, 'expected user object'
assert_not_nil resp_user['uuid'], 'expected user object'
assert_equal email, resp_user['email'], 'expected email not found'
-
end
def verify_link (link_object, link_class, link_name, tail_uuid, head_uuid)
@@ -650,7 +651,7 @@ class UserTest < ActiveSupport::TestCase
if not new_user_recipients.empty? then
assert_not_nil new_user_email, 'Expected new user email after setup'
assert_equal Rails.configuration.Users.UserNotifierEmailFrom, new_user_email.from[0]
- assert_equal new_user_recipients.keys.first, new_user_email.to[0]
+ assert_equal new_user_recipients.stringify_keys.keys.first, new_user_email.to[0]
assert_equal new_user_email_subject, new_user_email.subject
else
assert_nil new_user_email, 'Did not expect new user email after setup'
@@ -660,7 +661,7 @@ class UserTest < ActiveSupport::TestCase
if not inactive_recipients.empty? then
assert_not_nil new_inactive_user_email, 'Expected new inactive user email after setup'
assert_equal Rails.configuration.Users.UserNotifierEmailFrom, new_inactive_user_email.from[0]
- assert_equal inactive_recipients.keys.first, new_inactive_user_email.to[0]
+ assert_equal inactive_recipients.stringify_keys.keys.first, new_inactive_user_email.to[0]
assert_equal "#{Rails.configuration.Users.EmailSubjectPrefix}New inactive user notification", new_inactive_user_email.subject
else
assert_nil new_inactive_user_email, 'Did not expect new inactive user email after setup'
@@ -669,7 +670,6 @@ class UserTest < ActiveSupport::TestCase
assert_nil new_inactive_user_email, 'Expected no inactive user email after setting up active user'
end
ActionMailer::Base.deliveries = []
-
end
def verify_link_exists link_exists, head_uuid, tail_uuid, link_class, link_name, property_name=nil, property_value=nil
commit c986fef1ed1ad3967639f75d6e2d6ab0c63e4937
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Mon Sep 14 16:19:59 2020 -0300
16826: Enhances teardown config check.
It's not enough to check that .keys() returns a list of Symbols, because
ActiveRecord::OrderedOptions behaves somewhat like HashWithIndifferentAccess
so values can need to be accessed by symbol or string. So, we now check that
every hash-like config section is an OrderedOptions object and also returns
its keys as symbols.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index ad5fb8e2a..12e642d0e 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -99,20 +99,19 @@ class ActiveSupport::TestCase
end
end
- def confirm_keys_as_symbols(a_hash, section_name)
- a_hash.keys.each do |k|
- assert(k.is_a?(Symbol), "Key '#{k}' on section '#{section_name}' should be a Symbol")
- confirm_keys_as_symbols(a_hash[k], "#{section_name}.#{k}") if a_hash[k].is_a?(Hash)
+ def confirm_keys_as_symbols(conf, conf_name)
+ assert(conf.is_a?(ActiveSupport::OrderedOptions), "#{conf_name} should be an OrderedOptions object")
+ conf.keys.each do |k|
+ assert(k.is_a?(Symbol), "Key '#{k}' on section '#{conf_name}' should be a Symbol")
+ confirm_keys_as_symbols(conf[k], "#{conf_name}.#{k}") if conf[k].respond_to?('keys')
end
end
def restore_configuration
# Confirm that any changed configuration doesn't include non-symbol keys
- $arvados_config.keys.each do |config_section_name|
- config_section = Rails.configuration.send("#{config_section_name}")
- if config_section.is_a?(Hash)
- confirm_keys_as_symbols(config_section, config_section_name)
- end
+ $arvados_config.keys.each do |conf_name|
+ conf = Rails.configuration.send("#{conf_name}")
+ confirm_keys_as_symbols(conf, conf_name) if conf.respond_to?('keys')
end
# Restore configuration settings changed during tests
ConfigLoader.copy_into_config $arvados_config, Rails.configuration
commit 6350ddac78dbd2b29e5dc4117fd57300f7f8a098
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Mon Sep 14 15:42:15 2020 -0300
16826: Adds catch-all check for any config potentially changed during tests.
On test suite teardown, check if all config keys are still Symbols, so that
no mistake is made by writing tests that rely on config hashes with string
keys.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/lib/config_loader.rb b/services/api/lib/config_loader.rb
index cf16993ca..f421fb5b2 100644
--- a/services/api/lib/config_loader.rb
+++ b/services/api/lib/config_loader.rb
@@ -147,14 +147,14 @@ class ConfigLoader
'Ki' => 1 << 10,
'M' => 1000000,
'Mi' => 1 << 20,
- "G" => 1000000000,
- "Gi" => 1 << 30,
- "T" => 1000000000000,
- "Ti" => 1 << 40,
- "P" => 1000000000000000,
- "Pi" => 1 << 50,
- "E" => 1000000000000000000,
- "Ei" => 1 << 60,
+ "G" => 1000000000,
+ "Gi" => 1 << 30,
+ "T" => 1000000000000,
+ "Ti" => 1 << 40,
+ "P" => 1000000000000000,
+ "Pi" => 1 << 50,
+ "E" => 1000000000000000000,
+ "Ei" => 1 << 60,
}[mt[2]]
end
end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index c99a57aaf..ad5fb8e2a 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -99,7 +99,21 @@ class ActiveSupport::TestCase
end
end
+ def confirm_keys_as_symbols(a_hash, section_name)
+ a_hash.keys.each do |k|
+ assert(k.is_a?(Symbol), "Key '#{k}' on section '#{section_name}' should be a Symbol")
+ confirm_keys_as_symbols(a_hash[k], "#{section_name}.#{k}") if a_hash[k].is_a?(Hash)
+ end
+ end
+
def restore_configuration
+ # Confirm that any changed configuration doesn't include non-symbol keys
+ $arvados_config.keys.each do |config_section_name|
+ config_section = Rails.configuration.send("#{config_section_name}")
+ if config_section.is_a?(Hash)
+ confirm_keys_as_symbols(config_section, config_section_name)
+ end
+ end
# Restore configuration settings changed during tests
ConfigLoader.copy_into_config $arvados_config, Rails.configuration
ConfigLoader.copy_into_config $remaining_config, Rails.configuration
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list