From 57caf08e1331f7f22c2a0f532480347a8b13fff8 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 28 Sep 2022 11:48:16 +1000 Subject: [PATCH] DEV: Minimal first pass of rails system test setup (#16311) This commit introduces rails system tests run with chromedriver, selenium, and headless chrome to our testing toolbox. We use the `webdrivers` gem and `selenium-webdriver` which is what the latest Rails uses so the tests run locally and in CI out of the box. You can use `SELENIUM_VERBOSE_DRIVER_LOGS=1` to show extra verbose logs of what selenium is doing to communicate with the system tests. By default JS logs are verbose so errors from JS are shown when running system tests, you can disable this with `SELENIUM_DISABLE_VERBOSE_JS_LOGS=1` You can use `SELENIUM_HEADLESS=0` to run the system tests inside a chrome browser instead of headless, which can be useful to debug things and see what the spec sees. See note above about `bin/ember-cli` to avoid surprises. I have modified `bin/turbo_rspec` to exclude `spec/system` by default, support for parallel system specs is a little shaky right now and we don't want them slowing down the turbo by default either. ### PageObjects and System Tests To make querying and inspecting parts of the page easier and more reusable inbetween system tests, we are using the concept of [PageObjects](https://www.selenium.dev/documentation/test_practices/encouraged/page_object_models/) in our system tests. A "Page" here is generally corresponds to an overarching ember route, e.g. "Topic" for `/t/324345/some-topic`, and this contains logic for querying components within the topic such as "Posts". I have also split "Modals" into their own entity. Further down the line we may want to explore creating independent "Component" contexts. Capybara DSL should be included in each PageObject class, reference for this can be found at https://rubydoc.info/github/teamcapybara/capybara/master#the-dsl For system tests, since they are so slow, we want to focus on the "happy path" and not do every different possible context and branch check using them. They are meant to be overarching tests that check a number of things are correct using the full stack from JS and ember to rails to ruby and then the database. ### CI Setup Whenever a system spec fails, a screenshot is taken and a build artifact is produced _after the entire CI run is complete_, which can be downloaded from the Actions UI in the repo. Most importantly, a step to build the Ember app using Ember CLI is needed, otherwise the JS assets cannot be found by capybara: ``` - name: Build Ember CLI run: bin/ember-cli --build ``` A new `--build` argument has been added to `bin/ember-cli` for this case, which is not needed locally if you already have the discourse rails server running via `bin/ember-cli -u` since the whole server is built and set up by default. Co-authored-by: David Taylor --- .github/workflows/tests.yml | 25 ++++- Gemfile | 3 + Gemfile.lock | 26 +++++ bin/ember-cli | 6 +- bin/system_rspec | 7 ++ bin/turbo_rspec | 10 +- spec/rails_helper.rb | 108 ++++++++++++++++++-- spec/support/system_helpers.rb | 27 +++++ spec/system/bookmarks_spec.rb | 67 ++++++++++++ spec/system/page_objects/modals/base.rb | 18 ++++ spec/system/page_objects/modals/bookmark.rb | 19 ++++ spec/system/page_objects/pages/base.rb | 13 +++ spec/system/page_objects/pages/topic.rb | 60 +++++++++++ 13 files changed, 376 insertions(+), 13 deletions(-) create mode 100755 bin/system_rspec create mode 100644 spec/support/system_helpers.rb create mode 100644 spec/system/bookmarks_spec.rb create mode 100644 spec/system/page_objects/modals/base.rb create mode 100644 spec/system/page_objects/modals/bookmark.rb create mode 100644 spec/system/page_objects/pages/base.rb create mode 100644 spec/system/page_objects/pages/topic.rb diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e76e85f20d3..f1356bf73bd 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -19,7 +19,7 @@ jobs: build: name: ${{ matrix.target }} ${{ matrix.build_type }} runs-on: ${{ (matrix.build_type == 'annotations') && 'ubuntu-latest' || 'ubuntu-20.04-8core' }} - container: discourse/discourse_test:slim${{ startsWith(matrix.build_type, 'frontend') && '-browsers' || '' }} + container: discourse/discourse_test:slim${{ (matrix.build_type == 'frontend' || matrix.build_type == 'system') && '-browsers' || '' }} timeout-minutes: 20 env: @@ -34,13 +34,15 @@ jobs: fail-fast: false matrix: - build_type: [backend, frontend, annotations] + build_type: [backend, frontend, system, annotations] target: [core, plugins] exclude: - build_type: annotations target: plugins - build_type: frontend target: core # Handled by core_frontend_tests job (below) + - build_type: system + target: plugins # Enable once at least 1 plugin has system tests steps: - uses: actions/checkout@v3 @@ -166,6 +168,25 @@ jobs: run: QUNIT_PARALLEL=3 bin/rake plugin:qunit['*','1200000'] timeout-minutes: 30 + - name: Ember Build for System Tests + if: matrix.build_type == 'system' + run: bin/ember-cli --build + + - name: Core System Tests + if: matrix.build_type == 'system' && matrix.target == 'core' + run: bin/system_rspec + + - name: Plugin System Tests + if: matrix.build_type == 'system' && matrix.target == 'plugins' + run: bin/system_rspec plugins/*/spec/system + + - name: Upload failed system test screenshots + uses: actions/upload-artifact@v3 + if: matrix.build_type == 'system' && failure() + with: + name: failed-system-test-screenshots + path: tmp/screenshots/*.png + - name: Check Annotations if: matrix.build_type == 'annotations' run: | diff --git a/Gemfile b/Gemfile index a59ecb56409..e29c6737bed 100644 --- a/Gemfile +++ b/Gemfile @@ -149,11 +149,14 @@ group :assets do end group :test do + gem 'capybara', require: false gem 'webmock', require: false gem 'fakeweb', require: false gem 'minitest', require: false gem 'simplecov', require: false + gem 'selenium-webdriver', require: false gem "test-prof" + gem 'webdrivers', require: false end group :test, :development do diff --git a/Gemfile.lock b/Gemfile.lock index 51df0fa0eb2..f1eb7ed5b8b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -89,8 +89,18 @@ GEM activesupport (>= 3.0.0) uniform_notifier (~> 1.11) byebug (11.1.3) + capybara (3.37.1) + addressable + matrix + mini_mime (>= 0.1.3) + nokogiri (~> 1.8) + rack (>= 1.6.0) + rack-test (>= 0.6.3) + regexp_parser (>= 1.5, < 3.0) + xpath (~> 3.2) cbor (0.5.9.6) certified (1.0.0) + childprocess (4.1.0) chunky_png (1.4.0) coderay (1.1.3) colored2 (3.1.2) @@ -208,6 +218,7 @@ GEM nokogiri (>= 1.5.9) lru_redux (1.1.0) lz4-ruby (0.3.3) + matrix (0.4.2) maxminddb (0.1.22) memory_profiler (1.0.0) message_bus (4.2.0) @@ -434,6 +445,11 @@ GEM seed-fu (2.3.9) activerecord (>= 3.1) activesupport (>= 3.1) + selenium-webdriver (4.4.0) + childprocess (>= 0.5, < 5.0) + rexml (~> 3.2, >= 3.2.5) + rubyzip (>= 1.2.2, < 3.0) + websocket (~> 1.0) shoulda-matchers (5.2.0) activesupport (>= 5.2.0) sidekiq (6.5.7) @@ -478,6 +494,10 @@ GEM uri (0.11.0) uri_template (0.7.0) version_gem (1.1.0) + webdrivers (5.1.0) + nokogiri (~> 1.6) + rubyzip (>= 1.3.0) + selenium-webdriver (~> 4.0) webmock (3.18.1) addressable (>= 2.8.0) crack (>= 0.3.2) @@ -485,7 +505,10 @@ GEM webpush (1.1.0) hkdf (~> 0.2) jwt (~> 2.0) + websocket (1.2.9) xorcist (1.1.3) + xpath (3.2.0) + nokogiri (~> 1.8) yaml-lint (0.0.10) zeitwerk (2.6.0) @@ -517,6 +540,7 @@ DEPENDENCIES bootsnap bullet byebug + capybara cbor certified colored2 @@ -609,6 +633,7 @@ DEPENDENCIES sassc (= 2.0.1) sassc-rails seed-fu + selenium-webdriver shoulda-matchers sidekiq simplecov @@ -621,6 +646,7 @@ DEPENDENCIES uglifier unf unicorn + webdrivers webmock webpush xorcist diff --git a/bin/ember-cli b/bin/ember-cli index 7f5decb36fc..d1c480085cc 100755 --- a/bin/ember-cli +++ b/bin/ember-cli @@ -7,7 +7,7 @@ RAILS_ROOT = File.expand_path("../../", Pathname.new(__FILE__).realpath) PORT = ENV["UNICORN_PORT"] ||= "3000" HOSTNAME = ENV["DISCOURSE_HOSTNAME"] ||= "127.0.0.1" YARN_DIR = File.join(RAILS_ROOT, "app/assets/javascripts/discourse") -CUSTOM_ARGS = ["--try", "--test", "--unicorn", "-u", "--forward-host"] +CUSTOM_ARGS = ["--try", "--test", "--build", "--unicorn", "-u", "--forward-host"] PROXY = if ARGV.include?("--try") "https://try.discourse.org" @@ -24,6 +24,8 @@ end command = if ARGV.include?("--test") "test" + elsif ARGV.include?("--build") + "build" else "server" end @@ -49,7 +51,7 @@ end args = ["-s", "--cwd", YARN_DIR, "run", "ember", command] + (ARGV - CUSTOM_ARGS) -if !args.include?("test") && !args.include?("--proxy") +if !args.include?("test") && !args.include?("build") && !args.include?("--proxy") args << "--proxy" args << PROXY end diff --git a/bin/system_rspec b/bin/system_rspec new file mode 100755 index 00000000000..3f9e0f2adf3 --- /dev/null +++ b/bin/system_rspec @@ -0,0 +1,7 @@ +#!/bin/bash + +if [[ -z "$@" ]]; then + bin/rspec spec/system +elif [[ -n "$@" ]]; then + bin/rspec "$@" +fi diff --git a/bin/turbo_rspec b/bin/turbo_rspec index 66464bc67b5..7d4c4278163 100755 --- a/bin/turbo_rspec +++ b/bin/turbo_rspec @@ -59,10 +59,18 @@ formatters.each do |formatter| end end +# We do not want to include system specs by default, they are super +# slow and require a selenium server to be running. +default_spec_dirs = Dir.entries("#{Rails.root}/spec").reject do |entry| + !File.directory?("spec/#{entry}") || ["..", ".", "system"].include?(entry) +end.map { |entry| "spec/#{entry}" } +files = ARGV.empty? ? default_spec_dirs : ARGV + +puts "Running turbo_rspec using files in #{files}" success = TurboTests::Runner.run( formatters: formatters, - files: ARGV.empty? ? ["spec"] : ARGV, + files: files, verbose: verbose, fail_fast: fail_fast ) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 1704c0e23b0..e825618a942 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -57,6 +57,9 @@ require 'shoulda-matchers' require 'sidekiq/testing' require 'test_prof/recipes/rspec/let_it_be' require 'test_prof/before_all/adapters/active_record' +require 'webdrivers' +require 'selenium-webdriver' +require 'capybara/rails' # The shoulda-matchers gem no longer detects the test framework # you're using or mixes itself into that framework automatically. @@ -71,6 +74,11 @@ end # Requires supporting ruby files with custom matchers and macros, etc, # in spec/support/ and its subdirectories. Dir[Rails.root.join("spec/support/**/*.rb")].each { |f| require f } + +require Rails.root.join("spec/system/page_objects/pages/base.rb") +require Rails.root.join("spec/system/page_objects/modals/base.rb") +Dir[Rails.root.join("spec/system/page_objects/**/*.rb")].each { |f| require f } + Dir[Rails.root.join("spec/fabricators/*.rb")].each { |f| require f } require_relative './helpers/redis_snapshot_helper' @@ -83,6 +91,10 @@ if ENV['LOAD_PLUGINS'] == "1" Dir[Rails.root.join("plugins/*/spec/fabricators/**/*.rb")].each do |f| require f end + + Dir[Rails.root.join("plugins/*/spec/system/page_objects/**/*.rb")].each do |f| + require f + end end # let's not run seed_fu every test @@ -177,6 +189,7 @@ RSpec.configure do |config| config.include MessageBus config.include RSpecHtmlMatchers config.include IntegrationHelpers, type: :request + config.include SystemHelpers, type: :system config.include WebauthnIntegrationHelpers config.include SiteSettingsHelpers config.include SidekiqHelpers @@ -228,7 +241,41 @@ RSpec.configure do |config| SiteSetting.provider = TestLocalProcessProvider.new - WebMock.disable_net_connect! + WebMock.disable_net_connect!( + allow_localhost: true, + allow: [Webdrivers::Chromedriver.base_url] + ) + + Capybara.configure do |capybara_config| + capybara_config.server_host = "localhost" + capybara_config.server_port = 31337 + end + + chrome_browser_options = Selenium::WebDriver::Chrome::Options.new( + logging_prefs: { "browser" => "ALL", "driver" => "ALL" } + ).tap do |options| + options.add_argument("--window-size=1400,1400") + options.add_argument("--no-sandbox") + options.add_argument("--disable-dev-shm-usage") + end + + Capybara.register_driver :selenium_chrome do |app| + Capybara::Selenium::Driver.new( + app, + browser: :chrome, + capabilities: chrome_browser_options, + ) + end + + Capybara.register_driver :selenium_chrome_headless do |app| + chrome_browser_options.add_argument("--headless") + + Capybara::Selenium::Driver.new( + app, + browser: :chrome, + capabilities: chrome_browser_options, + ) + end if ENV['ELEVATED_UPLOADS_ID'] DB.exec "SELECT setval('uploads_id_seq', 10000)" @@ -246,14 +293,14 @@ RSpec.configure do |config| end end - config.after :each do |x| - if x.exception && ex = RspecErrorTracker.last_exception + config.after :each do |example| + if example.exception && ex = RspecErrorTracker.last_exception # magic in a cause if we have none - unless x.exception.cause - class << x.exception + unless example.exception.cause + class << example.exception attr_accessor :cause end - x.exception.cause = ex + example.exception.cause = ex end end @@ -286,8 +333,53 @@ RSpec.configure do |config| end # Match the request hostname to the value in `database.yml` - config.before(:all, type: [:request, :multisite]) { host! "test.localhost" } - config.before(:each, type: [:request, :multisite]) { host! "test.localhost" } + config.before(:all, type: [:request, :multisite, :system]) { host! "test.localhost" } + config.before(:each, type: [:request, :multisite, :system]) { host! "test.localhost" } + + last_driven_by = nil + config.before(:each, type: :system) do |example| + if example.metadata[:js] + driver = "selenium_chrome" + driver += "_headless" unless ENV["SELENIUM_HEADLESS"] == "0" + driven_by driver.to_sym + end + setup_system_test + end + + config.after(:each, type: :system) do |example| + # This is disabled by default because it is super verbose, + # if you really need to dig into how selenium is communicating + # for system tests then enable it. + if ENV["SELENIUM_VERBOSE_DRIVER_LOGS"] + puts "~~~~~~ DRIVER LOGS: ~~~~~~~" + page.driver.browser.logs.get(:driver).each do |log| + puts log.message + end + end + + # Recommended that this is not disabled, since it makes debugging + # failed system tests a lot trickier. + if ENV["SELENIUM_DISABLE_VERBOSE_JS_LOGS"].blank? + if example.exception + skip_js_errors = false + if example.exception.kind_of?(RSpec::Core::MultipleExceptionError) + puts "~~~~~~ SYSTEM TEST ERRORS: ~~~~~~~" + example.exception.all_exceptions.each do |ex| + puts ex.message + end + + skip_js_errors = true + end + + if !skip_js_errors + puts "~~~~~~ JS ERRORS: ~~~~~~~" + page.driver.browser.logs.get(:browser).each do |log| + puts log.message + end + end + end + end + end config.before(:each, type: :multisite) do Rails.configuration.multisite = true # rubocop:disable Discourse/NoDirectMultisiteManipulation diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb new file mode 100644 index 00000000000..b7116b2a4c4 --- /dev/null +++ b/spec/support/system_helpers.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module SystemHelpers + def sign_in(user) + visit "/session/#{user.encoded_username}/become" + end + + def setup_system_test + SiteSetting.login_required = false + SiteSetting.content_security_policy = false + SiteSetting.force_hostname = "#{Capybara.server_host}:#{Capybara.server_port}" + SiteSetting.external_system_avatars_enabled = false + end + + def try_until_success(timeout: 2, frequency: 0.01) + start ||= Time.zone.now + backoff ||= frequency + yield + rescue RSpec::Expectations::ExpectationNotMetError + if Time.zone.now >= start + timeout.seconds + raise + end + sleep backoff + backoff += frequency + retry + end +end diff --git a/spec/system/bookmarks_spec.rb b/spec/system/bookmarks_spec.rb new file mode 100644 index 00000000000..82802505a92 --- /dev/null +++ b/spec/system/bookmarks_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +describe "Bookmarking posts and topics", type: :system, js: true do + fab!(:topic) { Fabricate(:topic) } + fab!(:user) { Fabricate(:user, username: "bookmarkguy") } + fab!(:post) { Fabricate(:post, topic: topic, raw: "This is some post to bookmark") } + fab!(:post2) { Fabricate(:post, topic: topic, raw: "Some interesting post content") } + + it "allows logged in user to create bookmarks with and without reminders" do + sign_in user + visit "/t/#{topic.id}" + topic_page = PageObjects::Pages::Topic.new + expect(topic_page).to have_post_content(post) + topic_page.expand_post_actions(post) + topic_page.click_post_action_button(post, :bookmark) + + bookmark_modal = PageObjects::Modals::Bookmark.new + bookmark_modal.fill_name("something important") + bookmark_modal.save + + expect(topic_page).to have_post_bookmarked(post) + bookmark = Bookmark.find_by(bookmarkable: post, user: user) + expect(bookmark.name).to eq("something important") + + topic_page.expand_post_actions(post2) + topic_page.click_post_action_button(post2, :bookmark) + + bookmark_modal = PageObjects::Modals::Bookmark.new + bookmark_modal.select_preset_reminder(:tomorrow) + expect(topic_page).to have_post_bookmarked(post2) + bookmark = Bookmark.find_by(bookmarkable: post2, user: user) + expect(bookmark.reminder_at).not_to eq(nil) + expect(bookmark.reminder_set_at).not_to eq(nil) + end + + it "does not create a bookmark if the modal is closed with the cancel button" do + sign_in user + visit "/t/#{topic.id}" + topic_page = PageObjects::Pages::Topic.new + topic_page.expand_post_actions(post) + topic_page.click_post_action_button(post, :bookmark) + + bookmark_modal = PageObjects::Modals::Bookmark.new + bookmark_modal.fill_name("something important") + bookmark_modal.cancel + + expect(topic_page).not_to have_post_bookmarked(post) + expect(Bookmark.exists?(bookmarkable: post, user: user)).to eq(false) + end + + it "allows the topic to be bookmarked" do + sign_in user + visit "/t/#{topic.id}" + topic_page = PageObjects::Pages::Topic.new + topic_page.click_topic_footer_button(:bookmark) + + bookmark_modal = PageObjects::Modals::Bookmark.new + bookmark_modal.fill_name("something important") + bookmark_modal.save + + expect(topic_page).to have_topic_bookmarked + bookmark = try_until_success do + expect(Bookmark.exists?(bookmarkable: topic, user: user)).to eq(true) + end + expect(bookmark).not_to eq(nil) + end +end diff --git a/spec/system/page_objects/modals/base.rb b/spec/system/page_objects/modals/base.rb new file mode 100644 index 00000000000..1e7f477a859 --- /dev/null +++ b/spec/system/page_objects/modals/base.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module PageObjects + module Modals + class Base + include Capybara::DSL + include RSpec::Matchers + + def close + find(".modal-close").click + end + + def cancel + find(".d-modal-cancel").click + end + end + end +end diff --git a/spec/system/page_objects/modals/bookmark.rb b/spec/system/page_objects/modals/bookmark.rb new file mode 100644 index 00000000000..c9d3ac6c946 --- /dev/null +++ b/spec/system/page_objects/modals/bookmark.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module PageObjects + module Modals + class Bookmark < PageObjects::Modals::Base + def fill_name(name) + fill_in "bookmark-name", with: name + end + + def select_preset_reminder(identifier) + find("#tap_tile_#{identifier}").click + end + + def save + find("#save-bookmark").click + end + end + end +end diff --git a/spec/system/page_objects/pages/base.rb b/spec/system/page_objects/pages/base.rb new file mode 100644 index 00000000000..60c2ffb24f2 --- /dev/null +++ b/spec/system/page_objects/pages/base.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module PageObjects + module Pages + class Base + include Capybara::DSL + + def setup_component_classes!(component_classes) + @component_classes = component_classes + end + end + end +end diff --git a/spec/system/page_objects/pages/topic.rb b/spec/system/page_objects/pages/topic.rb new file mode 100644 index 00000000000..aff0dc4ccfb --- /dev/null +++ b/spec/system/page_objects/pages/topic.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module PageObjects + module Pages + class Topic < PageObjects::Pages::Base + def initialize + setup_component_classes!( + post_show_more_actions: ".show-more-actions", + post_action_button_bookmark: ".bookmark.with-reminder" + ) + end + + def has_post_content?(post) + post_by_number(post).has_content? post.raw + end + + def has_post_more_actions?(post) + within post_by_number(post) do + has_css?(@component_classes[:post_show_more_actions]) + end + end + + def has_post_bookmarked?(post) + within post_by_number(post) do + has_css?(@component_classes[:post_action_button_bookmark] + ".bookmarked") + end + end + + def expand_post_actions(post) + post_by_number(post).find(@component_classes[:post_show_more_actions]).click + end + + def click_post_action_button(post, button) + post_by_number(post).find(@component_classes["post_action_button_#{button}".to_sym]).click + end + + def click_topic_footer_button(button) + find_topic_footer_button(button).click + end + + def has_topic_bookmarked? + has_css?("#{topic_footer_button_id("bookmark")}.bookmarked", text: "Edit Bookmark") + end + + def find_topic_footer_button(button) + find(topic_footer_button_id(button)) + end + + private + + def topic_footer_button_id(button) + "#topic-footer-button-#{button}" + end + + def post_by_number(post) + find("#post_#{post.post_number}") + end + end + end +end