From 57fcdc402d73ffd2cad776fa2ba61fa2c7afe466 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 17 Sep 2021 02:32:08 +0200 Subject: [PATCH 1/4] Use page.find instead of within in component tests In component tests, the `within` method is actually an alias to RSpec's `be_within` matcher, which is used to test numeric ranges. That meant the tests always passed, even when there were bugs on the page. In order to use `within` in component tests, we have to use `page.within`. However, that also fails, since there's no such method for `Capybara::Node::Simple'` objects, which are used in component tests. So we're using `page.find` instead. See also pull request 945 in https://github.com/github/view_component --- .../headings/group_switcher_component_spec.rb | 6 +++--- spec/components/budgets/budget_component_spec.rb | 8 ++++---- spec/components/budgets/subheader_component_spec.rb | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/components/admin/budgets_wizard/headings/group_switcher_component_spec.rb b/spec/components/admin/budgets_wizard/headings/group_switcher_component_spec.rb index 0392290f7..a0e0ee1cb 100644 --- a/spec/components/admin/budgets_wizard/headings/group_switcher_component_spec.rb +++ b/spec/components/admin/budgets_wizard/headings/group_switcher_component_spec.rb @@ -33,9 +33,9 @@ describe Admin::BudgetsWizard::Headings::GroupSwitcherComponent do expect(page).to have_content "Showing headings from the Parks group" expect(page).to have_button "Manage headings from a different group" - within "button + ul" do - expect(page).to have_link "Recreation" - expect(page).to have_link "Entertainment" + page.find("button + ul") do |list| + expect(list).to have_link "Recreation" + expect(list).to have_link "Entertainment" end end end diff --git a/spec/components/budgets/budget_component_spec.rb b/spec/components/budgets/budget_component_spec.rb index ad01643ae..98c0ef338 100644 --- a/spec/components/budgets/budget_component_spec.rb +++ b/spec/components/budgets/budget_component_spec.rb @@ -13,10 +13,10 @@ describe Budgets::BudgetComponent do render_inline Budgets::BudgetComponent.new(budget) - within(".budget-header") do - expect(page).to have_content("PARTICIPATORY BUDGETS") - expect(page).to have_content(budget.name) - expect(page).to have_link("Help with participatory budgets") + page.find(".budget-header") do |header| + expect(header).to have_content "Participatory budgets" + expect(header).to have_content budget.name + expect(header).to have_link "Help with participatory budgets" end end diff --git a/spec/components/budgets/subheader_component_spec.rb b/spec/components/budgets/subheader_component_spec.rb index 5ff750e71..cfccf8b83 100644 --- a/spec/components/budgets/subheader_component_spec.rb +++ b/spec/components/budgets/subheader_component_spec.rb @@ -7,9 +7,9 @@ describe Budgets::SubheaderComponent do render_inline Budgets::SubheaderComponent.new(budget) - within(".budget-subheader") do - expect(page).to have_content "CURRENT PHASE" - expect(page).to have_content "Information" + page.find(".budget-subheader") do |subheader| + expect(subheader).to have_content "Current phase" + expect(subheader).to have_content "Information" end end From d7f26f012d1db0f2b6938a2aadce99980c634f30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 Oct 2021 15:57:12 +0200 Subject: [PATCH 2/4] Fix test checking link in budget header The test was passing because we were using `within`, but actually the `have_css` method doesn't support the `href:` argument. --- spec/components/budgets/budget_component_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/components/budgets/budget_component_spec.rb b/spec/components/budgets/budget_component_spec.rb index 98c0ef338..95fd023c7 100644 --- a/spec/components/budgets/budget_component_spec.rb +++ b/spec/components/budgets/budget_component_spec.rb @@ -23,16 +23,16 @@ describe Budgets::BudgetComponent do it "shows budget main link when defined" do render_inline Budgets::BudgetComponent.new(budget) - within(".budget-header") do - expect(page).not_to have_css("a.main-link") + page.find(".budget-header") do |header| + expect(header).not_to have_css ".main-link" end - budget.update!(main_link_text: "Partitipate now!", main_link_url: "https://consulproject.org") + budget.update!(main_link_text: "Participate now!", main_link_url: "https://consulproject.org") render_inline Budgets::BudgetComponent.new(budget) - within(".budget-header") do - expect(page).to have_css("a.main-link", text: "Participate now!", href: "https://consulproject.org") + page.find(".budget-header") do |header| + expect(header).to have_link "Participate now!", href: "https://consulproject.org", class: "main-link" end end end From 7154228fbb072f1bc3d8386725cf55e712ba8fe3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 Oct 2021 16:17:05 +0200 Subject: [PATCH 3/4] Move budget map icon tests back to system specs These tests don't work without JavaScript. They were passing because the `within` method always passes in component tests. This reverts most of commit 822140a14. --- .../budgets/budget_component_spec.rb | 90 ------------------ spec/system/budgets/budgets_spec.rb | 92 +++++++++++++++++++ 2 files changed, 92 insertions(+), 90 deletions(-) diff --git a/spec/components/budgets/budget_component_spec.rb b/spec/components/budgets/budget_component_spec.rb index 95fd023c7..02d94bc6a 100644 --- a/spec/components/budgets/budget_component_spec.rb +++ b/spec/components/budgets/budget_component_spec.rb @@ -36,94 +36,4 @@ describe Budgets::BudgetComponent do end end end - - describe "map" do - before do - Setting["feature.map"] = true - end - - it "displays investment's map location markers" do - investment1 = create(:budget_investment, heading: heading) - investment2 = create(:budget_investment, heading: heading) - investment3 = create(:budget_investment, heading: heading) - - create(:map_location, longitude: 40.1234, latitude: -3.634, investment: investment1) - create(:map_location, longitude: 40.1235, latitude: -3.635, investment: investment2) - create(:map_location, longitude: 40.1236, latitude: -3.636, investment: investment3) - - render_inline Budgets::BudgetComponent.new(budget) - - within ".map_location" do - expect(page).to have_css(".map-icon", count: 3) - end - end - - it "displays all investment's map location if there are no selected" do - budget.update!(phase: :publishing_prices) - - investment1 = create(:budget_investment, heading: heading) - investment2 = create(:budget_investment, heading: heading) - investment3 = create(:budget_investment, heading: heading) - investment4 = create(:budget_investment, heading: heading) - - investment1.create_map_location(longitude: 40.1234, latitude: 3.1234, zoom: 10) - investment2.create_map_location(longitude: 40.1235, latitude: 3.1235, zoom: 10) - investment3.create_map_location(longitude: 40.1236, latitude: 3.1236, zoom: 10) - investment4.create_map_location(longitude: 40.1240, latitude: 3.1240, zoom: 10) - - render_inline Budgets::BudgetComponent.new(budget) - - within ".map_location" do - expect(page).to have_css(".map-icon", count: 4) - end - end - - it "displays only selected investment's map location from publishing prices phase" do - budget.update!(phase: :publishing_prices) - - investment1 = create(:budget_investment, :selected, heading: heading) - investment2 = create(:budget_investment, :selected, heading: heading) - investment3 = create(:budget_investment, heading: heading) - investment4 = create(:budget_investment, heading: heading) - - investment1.create_map_location(longitude: 40.1234, latitude: 3.1234, zoom: 10) - investment2.create_map_location(longitude: 40.1235, latitude: 3.1235, zoom: 10) - investment3.create_map_location(longitude: 40.1236, latitude: 3.1236, zoom: 10) - investment4.create_map_location(longitude: 40.1240, latitude: 3.1240, zoom: 10) - - render_inline Budgets::BudgetComponent.new(budget) - - within ".map_location" do - expect(page).to have_css(".map-icon", count: 2) - end - end - - scenario "skips invalid map markers" do - map_locations = [] - - investment = create(:budget_investment, heading: heading) - - map_locations << { longitude: 40.123456789, latitude: 3.12345678 } - map_locations << { longitude: 40.123456789, latitude: "********" } - map_locations << { longitude: "**********", latitude: 3.12345678 } - - coordinates = map_locations.map do |map_location| - { - lat: map_location[:latitude], - long: map_location[:longitude], - investment_title: investment.title, - investment_id: investment.id, - budget_id: budget.id - } - end - - allow_any_instance_of(Budgets::BudgetComponent).to receive(:coordinates).and_return(coordinates) - - render_inline Budgets::BudgetComponent.new(budget) - - within ".map_location" do - expect(page).to have_css(".map-icon", count: 1) - end - end - end end diff --git a/spec/system/budgets/budgets_spec.rb b/spec/system/budgets/budgets_spec.rb index 6848923a9..01ae866a8 100644 --- a/spec/system/budgets/budgets_spec.rb +++ b/spec/system/budgets/budgets_spec.rb @@ -232,6 +232,98 @@ describe "Budgets" do expect(page).to have_css(".tabs-panel.is-active", count: 1) end + context "Index map" do + let(:heading) { create(:budget_heading, budget: budget) } + + before do + Setting["feature.map"] = true + end + + scenario "Display investment's map location markers" do + investment1 = create(:budget_investment, heading: heading) + investment2 = create(:budget_investment, heading: heading) + investment3 = create(:budget_investment, heading: heading) + + create(:map_location, longitude: 40.1234, latitude: -3.634, investment: investment1) + create(:map_location, longitude: 40.1235, latitude: -3.635, investment: investment2) + create(:map_location, longitude: 40.1236, latitude: -3.636, investment: investment3) + + visit budgets_path + + within ".map_location" do + expect(page).to have_css(".map-icon", count: 3, visible: :all) + end + end + + scenario "Display all investment's map location if there are no selected" do + budget.update!(phase: :publishing_prices) + + investment1 = create(:budget_investment, heading: heading) + investment2 = create(:budget_investment, heading: heading) + investment3 = create(:budget_investment, heading: heading) + investment4 = create(:budget_investment, heading: heading) + + investment1.create_map_location(longitude: 40.1234, latitude: 3.1234, zoom: 10) + investment2.create_map_location(longitude: 40.1235, latitude: 3.1235, zoom: 10) + investment3.create_map_location(longitude: 40.1236, latitude: 3.1236, zoom: 10) + investment4.create_map_location(longitude: 40.1240, latitude: 3.1240, zoom: 10) + + visit budgets_path + + within ".map_location" do + expect(page).to have_css(".map-icon", count: 4, visible: :all) + end + end + + scenario "Display only selected investment's map location from publishing prices phase" do + budget.update!(phase: :publishing_prices) + + investment1 = create(:budget_investment, :selected, heading: heading) + investment2 = create(:budget_investment, :selected, heading: heading) + investment3 = create(:budget_investment, heading: heading) + investment4 = create(:budget_investment, heading: heading) + + investment1.create_map_location(longitude: 40.1234, latitude: 3.1234, zoom: 10) + investment2.create_map_location(longitude: 40.1235, latitude: 3.1235, zoom: 10) + investment3.create_map_location(longitude: 40.1236, latitude: 3.1236, zoom: 10) + investment4.create_map_location(longitude: 40.1240, latitude: 3.1240, zoom: 10) + + visit budgets_path + + within ".map_location" do + expect(page).to have_css(".map-icon", count: 2, visible: :all) + end + end + + scenario "Skip invalid map markers" do + map_locations = [] + + investment = create(:budget_investment, heading: heading) + + map_locations << { longitude: 40.123456789, latitude: 3.12345678 } + map_locations << { longitude: 40.123456789, latitude: "********" } + map_locations << { longitude: "**********", latitude: 3.12345678 } + + coordinates = map_locations.map do |map_location| + { + lat: map_location[:latitude], + long: map_location[:longitude], + investment_title: investment.title, + investment_id: investment.id, + budget_id: budget.id + } + end + + allow_any_instance_of(Budgets::BudgetComponent).to receive(:coordinates).and_return(coordinates) + + visit budgets_path + + within ".map_location" do + expect(page).to have_css(".map-icon", count: 1, visible: :all) + end + end + end + context "Show" do let!(:budget) { create(:budget, :selecting) } let!(:group) { create(:budget_group, budget: budget) } From 174f76507482d1ca31db85e03722d362c793060c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 Oct 2021 16:29:43 +0200 Subject: [PATCH 4/4] Make component tests fail when using `within` This way we avoid writing useless tests which always pass. --- spec/rails_helper.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 874273bad..6714ebdb4 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -18,6 +18,10 @@ module ViewComponent def sign_in(user) allow(controller).to receive(:current_user).and_return(user) end + + def within(...) + raise "`within` doesn't work in component tests. Use `page.find` instead." + end end end