Project

General

Profile

Actions
Copy link

Bug #16512

closed

Settings-related functional test failures

Added by Dominic Cleal over 7 years ago. Updated almost 6 years ago.

Status:
Closed
Priority:
High
Assignee:
Dominic Cleal
Category:
Tests
Target version:
1.14.0
Difficulty:
Triaged:
Bugzilla link:
Pull request:
https://github.com/theforeman/foreman/pull/3832
Fixed in Releases:
Found in Releases:
Nightly
Red Hat JIRA:

Description

Since #16491, 18 tests have started failing:

  1) Error:
SettingsControllerTest#test_0005_settings shouldnt include ones about locations when locations are disabled:
ActionView::Template::Error: undefined method `each' for nil:NilClass
    app/views/settings/index.html.erb:4:in `_app_views_settings_index_html_erb__462412887402587618_117671740'
    app/controllers/concerns/application_shared.rb:14:in `set_timezone'
    app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
    test/functional/settings_controller_test.rb:49:in `block in <class:SettingsControllerTest>'
  • Api::V1::SettingsControllerTest.test_0004_should parse string values
  • Api::V1::SettingsControllerTest.test_0002_should show individual record
  • Api::V1::SettingsControllerTest.test_0003_should update setting
  • Api::V2::SettingsControllerTest.test_0005_settings shouldnt include ones about organizations when organizations are disabled
  • Api::V2::SettingsControllerTest.test_0003_should not update setting
  • Api::V2::SettingsControllerTest.test_0006_settings shouldnt include ones about locations when locations are disabled
  • Api::V2::SettingsControllerTest.test_0001_should get index
  • Api::V2::SettingsControllerTest.test_0004_should parse string values
  • Api::V2::SettingsControllerTest.test_0002_should show individual record
  • Api::V2::UsersControllerTest.test_0005_shows default taxonomies on show response
  • Api::V2::UsersControllerTest.test_0002_should handle taxonomy with wrong id
  • LocationsControllerTest.test_0008_should display a warning if current location has been deleted
  • OrganizationsControllerTest.test_0009_should display a warning if current organization has been deleted
  • SettingsControllerTest.test_index
  • SettingsControllerTest.test_0002_does not render an old sti type setting
  • SettingsControllerTest.test_0005_settings shouldnt include ones about locations when locations are disabled
  • SettingsControllerTest.test_0004_settings shouldnt include ones about organizations when organizations are disabled
  • SettingsControllerTest.test_0001_can render a new sti type setting

Related issues 1 (0 open — 1 closed)

Related to Foreman - Bug #16491: upgrade rubocop to version 0.42ClosedTomer Brisker09/08/2016Actions
Actions
Copy link
 #1

Updated by Dominic Cleal over 7 years ago

  • Related to Bug #16491: upgrade rubocop to version 0.42 added
Actions
Copy link
 #2

Updated by Dominic Cleal over 7 years ago

  • Description updated (diff)
Actions
Copy link
 #3

Updated by Dominic Cleal over 7 years ago

The implementation of respond_to_missing? on Setting (and possibly other models) has caused a change of behaviour with group_by on AR collections. Previously:

> Setting::General.all.group_by(&:category)
2016-09-12T11:32:39  [sql] [D]   Setting::General Load (0.2ms)  SELECT "settings".* FROM "settings" WHERE "settings"."category" IN ('Setting::General')  ORDER BY "settings"."name" ASC
=> {"Setting::General"= ....

Now:

> Setting::General.all.group_by(&:category)
2016-09-12T11:31:08 0c55c282 [sql] [D]   Setting::General Load (0.2ms)  SELECT  "settings".* FROM "settings" WHERE "settings"."category" IN ('Setting::General') AND "settings"."name" = ?  ORDER BY "settings"."name" ASC LIMIT 1  [["name", "group_by"]]
=> nil
Actions
Copy link
 #4

Updated by Dominic Cleal over 7 years ago

  • Status changed from New to Assigned
  • Assignee set to Dominic Cleal
Actions
Copy link
 #5

Updated by The Foreman Bot over 7 years ago

  • Status changed from Assigned to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/3832 added
Actions
Copy link
 #6

Updated by Dominic Cleal over 7 years ago

  • Status changed from Ready For Testing to Closed
  • % Done changed from 0 to 100
Actions
Copy link
 #7

Updated by Dominic Cleal over 7 years ago

A little more detail: Setting started returning true for .respond_to?(:group_by) after respond_to_missing? was implemented, indicating that you could call Setting.group_by (which just returned nil as there's no such setting).

When calling Setting.all.group_by, Rails prefers the method on the model over its own method as it's assumed to be a scope: https://github.com/rails/rails/blob/v4.2.7.1/activerecord/lib/active_record/relation/delegation.rb#L92.

Ideally respond_to? should only respond true for known settings, but performing a DB query here would be unnecessarily expensive. My commit removed the (correct) respond_to_missing? implementation and just deprecated the method_missing behaviour so it can all be removed in time.

Actions
Copy link

Also available in: Atom PDF