diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index eb96d3cd2..d94218e9f 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -14,7 +14,7 @@ Before engaging with this community, please read and understand our ## Setting up development environment * The gem follows standard Rails practices: - * `bundle install` to install dependencies + * `bin/setup` to install dependencies * `bin/rails server` to start the server * `bin/rails test` to run tests * `bin/rails test:system` to run system tests diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d662285f3..a10e6a499 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,22 +6,26 @@ jobs: build: runs-on: ubuntu-latest name: Ruby ${{ matrix.ruby }} / ${{ matrix.gemfile }} / ${{ matrix.database }} + timeout-minutes: 5 strategy: fail-fast: false matrix: gemfile: [Gemfile, gemfiles/rails_6_0.gemfile, gemfiles/rails_6_1.gemfile, gemfiles/rails_7_0.gemfile, gemfiles/rails_edge.gemfile] - ruby: ["3.0", "3.1", "3.2"] + ruby: ["3.0", "3.1", "3.2", "3.3"] database: [sqlite] include: - gemfile: "gemfiles/postgresql.gemfile" ruby: 3.2 database: postgres + exclude: + - gemfile: "gemfiles/rails_edge.gemfile" + ruby: 3.0 env: BUNDLE_GEMFILE: ${{ matrix.gemfile }} DB: ${{ matrix.database }} steps: - name: Check out code - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Ruby ${{ matrix.ruby }} uses: ruby/setup-ruby@v1 with: @@ -47,8 +51,9 @@ jobs: - name: RuboCop run: bundle exec rubocop - name: Archive system test artifacts - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 if: failure() with: - name: screenshots + name: screenshots-${{ strategy.job-index }} path: test/dummy/tmp/screenshots + if-no-files-found: ignore diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml new file mode 100644 index 000000000..cb8b4d94f --- /dev/null +++ b/.github/workflows/stale.yml @@ -0,0 +1,26 @@ +name: "Close stale issues" +on: + schedule: + - cron: "30 1 * * *" + +permissions: + issues: write + pull-requests: write + +jobs: + stale: + runs-on: ubuntu-latest + steps: + - uses: actions/stale@v9 + with: + days-before-issue-stale: 60 + exempt-issue-labels: "Blocked" + days-before-issue-close: 14 + stale-issue-label: "stale" + stale-issue-message: > + This issue has been marked as stale because it has not been commented on in two months. + + Please reply in order to keep the issue open. Otherwise, it will close in 14 days. + + Thank you for contributing! + diff --git a/.ruby-version b/.ruby-version index e4604e3af..15a279981 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -3.2.1 +3.3.0 diff --git a/Gemfile b/Gemfile index c1b970bf2..0945ab377 100644 --- a/Gemfile +++ b/Gemfile @@ -8,8 +8,6 @@ gem "better_html" gem "capybara" gem "debug" gem "mocha" -gem "net-http" # Ruby 2.7 stdlib's net/http loads net/protocol relatively, which loads both the stdlib and gem version -gem "net-smtp" # mail is missing a dependency on net-smtp https://github.com/mikel/mail/pull/1439 gem "puma" if defined?(@rails_gem_requirement) && @rails_gem_requirement # causes Dependabot to ignore the next line and update the next gem "rails" @@ -20,8 +18,7 @@ else end gem "rubocop" gem "rubocop-shopify" -gem "selenium-webdriver", "< 4.10.1" +gem "selenium-webdriver" gem "sprockets-rails" gem "sqlite3" -gem "webdrivers", require: false gem "yard" diff --git a/Gemfile.lock b/Gemfile.lock index cb19f4267..9e3a12d4c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,84 +1,95 @@ PATH remote: . specs: - maintenance_tasks (2.1.1) + maintenance_tasks (2.4.0) actionpack (>= 6.0) activejob (>= 6.0) activerecord (>= 6.0) - job-iteration (~> 1.3.6) + job-iteration (>= 1.3.6) railties (>= 6.0) + zeitwerk (>= 2.6.2) GEM remote: https://rubygems.org/ specs: - actioncable (7.0.6) - actionpack (= 7.0.6) - activesupport (= 7.0.6) + actioncable (7.1.3) + actionpack (= 7.1.3) + activesupport (= 7.1.3) nio4r (~> 2.0) websocket-driver (>= 0.6.1) - actionmailbox (7.0.6) - actionpack (= 7.0.6) - activejob (= 7.0.6) - activerecord (= 7.0.6) - activestorage (= 7.0.6) - activesupport (= 7.0.6) + zeitwerk (~> 2.6) + actionmailbox (7.1.3) + actionpack (= 7.1.3) + activejob (= 7.1.3) + activerecord (= 7.1.3) + activestorage (= 7.1.3) + activesupport (= 7.1.3) mail (>= 2.7.1) net-imap net-pop net-smtp - actionmailer (7.0.6) - actionpack (= 7.0.6) - actionview (= 7.0.6) - activejob (= 7.0.6) - activesupport (= 7.0.6) + actionmailer (7.1.3) + actionpack (= 7.1.3) + actionview (= 7.1.3) + activejob (= 7.1.3) + activesupport (= 7.1.3) mail (~> 2.5, >= 2.5.4) net-imap net-pop net-smtp - rails-dom-testing (~> 2.0) - actionpack (7.0.6) - actionview (= 7.0.6) - activesupport (= 7.0.6) - rack (~> 2.0, >= 2.2.4) + rails-dom-testing (~> 2.2) + actionpack (7.1.3) + actionview (= 7.1.3) + activesupport (= 7.1.3) + nokogiri (>= 1.8.5) + racc + rack (>= 2.2.4) + rack-session (>= 1.0.1) rack-test (>= 0.6.3) - rails-dom-testing (~> 2.0) - rails-html-sanitizer (~> 1.0, >= 1.2.0) - actiontext (7.0.6) - actionpack (= 7.0.6) - activerecord (= 7.0.6) - activestorage (= 7.0.6) - activesupport (= 7.0.6) + rails-dom-testing (~> 2.2) + rails-html-sanitizer (~> 1.6) + actiontext (7.1.3) + actionpack (= 7.1.3) + activerecord (= 7.1.3) + activestorage (= 7.1.3) + activesupport (= 7.1.3) globalid (>= 0.6.0) nokogiri (>= 1.8.5) - actionview (7.0.6) - activesupport (= 7.0.6) + actionview (7.1.3) + activesupport (= 7.1.3) builder (~> 3.1) - erubi (~> 1.4) - rails-dom-testing (~> 2.0) - rails-html-sanitizer (~> 1.1, >= 1.2.0) - activejob (7.0.6) - activesupport (= 7.0.6) + erubi (~> 1.11) + rails-dom-testing (~> 2.2) + rails-html-sanitizer (~> 1.6) + activejob (7.1.3) + activesupport (= 7.1.3) globalid (>= 0.3.6) - activemodel (7.0.6) - activesupport (= 7.0.6) - activerecord (7.0.6) - activemodel (= 7.0.6) - activesupport (= 7.0.6) - activestorage (7.0.6) - actionpack (= 7.0.6) - activejob (= 7.0.6) - activerecord (= 7.0.6) - activesupport (= 7.0.6) + activemodel (7.1.3) + activesupport (= 7.1.3) + activerecord (7.1.3) + activemodel (= 7.1.3) + activesupport (= 7.1.3) + timeout (>= 0.4.0) + activestorage (7.1.3) + actionpack (= 7.1.3) + activejob (= 7.1.3) + activerecord (= 7.1.3) + activesupport (= 7.1.3) marcel (~> 1.0) - mini_mime (>= 1.1.0) - activesupport (7.0.6) + activesupport (7.1.3) + base64 + bigdecimal concurrent-ruby (~> 1.0, >= 1.0.2) + connection_pool (>= 2.2.5) + drb i18n (>= 1.6, < 2) minitest (>= 5.1) + mutex_m tzinfo (~> 2.0) - addressable (2.8.4) + addressable (2.8.6) public_suffix (>= 2.0.2, < 6.0) ast (2.4.2) + base64 (0.2.0) better_html (2.0.2) actionview (>= 6.0) activesupport (>= 6.0) @@ -86,35 +97,40 @@ GEM erubi (~> 1.4) parser (>= 2.4) smart_properties + bigdecimal (3.1.6) builder (3.2.4) - capybara (3.39.2) + capybara (3.40.0) addressable matrix mini_mime (>= 0.1.3) - nokogiri (~> 1.8) + nokogiri (~> 1.11) rack (>= 1.6.0) rack-test (>= 0.6.3) regexp_parser (>= 1.5, < 3.0) xpath (~> 3.2) - concurrent-ruby (1.2.2) + concurrent-ruby (1.2.3) + connection_pool (2.4.1) crass (1.0.6) - date (3.3.3) - debug (1.8.0) - irb (>= 1.5.0) - reline (>= 0.3.1) + date (3.3.4) + debug (1.9.1) + irb (~> 1.10) + reline (>= 0.3.8) + drb (2.2.0) + ruby2_keywords erubi (1.12.0) - globalid (1.1.0) - activesupport (>= 5.0) + globalid (1.2.1) + activesupport (>= 6.1) i18n (1.14.1) concurrent-ruby (~> 1.0) - io-console (0.6.0) - irb (1.6.4) - reline (>= 0.3.0) - job-iteration (1.3.6) + io-console (0.7.2) + irb (1.11.1) + rdoc + reline (>= 0.4.2) + job-iteration (1.4.1) activejob (>= 5.2) - json (2.6.3) + json (2.7.1) language_server-protocol (3.17.0.3) - loofah (2.21.3) + loofah (2.22.0) crass (~> 1.0.2) nokogiri (>= 1.12.0) mail (2.8.1) @@ -124,97 +140,106 @@ GEM net-smtp marcel (1.0.2) matrix (0.4.2) - method_source (1.0.0) - mini_mime (1.1.2) - mini_portile2 (2.8.2) - minitest (5.18.1) - mocha (2.0.4) + mini_mime (1.1.5) + mini_portile2 (2.8.5) + minitest (5.21.2) + mocha (2.1.0) ruby2_keywords (>= 0.0.5) - net-http (0.3.2) - uri - net-imap (0.3.6) + mutex_m (0.2.0) + net-imap (0.4.9.1) date net-protocol net-pop (0.1.2) net-protocol - net-protocol (0.2.1) + net-protocol (0.2.2) timeout - net-smtp (0.3.3) + net-smtp (0.4.0.1) net-protocol - nio4r (2.5.9) - nokogiri (1.15.2) + nio4r (2.7.0) + nokogiri (1.16.0) mini_portile2 (~> 2.8.2) racc (~> 1.4) - nokogiri (1.15.2-arm64-darwin) + nokogiri (1.16.0-arm64-darwin) racc (~> 1.4) - nokogiri (1.15.2-x86_64-darwin) + nokogiri (1.16.0-x86_64-darwin) racc (~> 1.4) - nokogiri (1.15.2-x86_64-linux) + nokogiri (1.16.0-x86_64-linux) racc (~> 1.4) - parallel (1.23.0) - parser (3.2.2.3) + parallel (1.24.0) + parser (3.3.0.5) ast (~> 2.4.1) racc - public_suffix (5.0.1) - puma (6.3.0) + psych (5.1.2) + stringio + public_suffix (5.0.4) + puma (6.4.2) nio4r (~> 2.0) - racc (1.7.1) - rack (2.2.7) + racc (1.7.3) + rack (2.2.8) + rack-session (1.0.2) + rack (< 3) rack-test (2.1.0) rack (>= 1.3) - rails (7.0.6) - actioncable (= 7.0.6) - actionmailbox (= 7.0.6) - actionmailer (= 7.0.6) - actionpack (= 7.0.6) - actiontext (= 7.0.6) - actionview (= 7.0.6) - activejob (= 7.0.6) - activemodel (= 7.0.6) - activerecord (= 7.0.6) - activestorage (= 7.0.6) - activesupport (= 7.0.6) + rackup (1.0.0) + rack (< 3) + webrick + rails (7.1.3) + actioncable (= 7.1.3) + actionmailbox (= 7.1.3) + actionmailer (= 7.1.3) + actionpack (= 7.1.3) + actiontext (= 7.1.3) + actionview (= 7.1.3) + activejob (= 7.1.3) + activemodel (= 7.1.3) + activerecord (= 7.1.3) + activestorage (= 7.1.3) + activesupport (= 7.1.3) bundler (>= 1.15.0) - railties (= 7.0.6) - rails-dom-testing (2.1.1) + railties (= 7.1.3) + rails-dom-testing (2.2.0) activesupport (>= 5.0.0) minitest nokogiri (>= 1.6) rails-html-sanitizer (1.6.0) loofah (~> 2.21) nokogiri (~> 1.14) - railties (7.0.6) - actionpack (= 7.0.6) - activesupport (= 7.0.6) - method_source + railties (7.1.3) + actionpack (= 7.1.3) + activesupport (= 7.1.3) + irb + rackup (>= 1.0.0) rake (>= 12.2) - thor (~> 1.0) - zeitwerk (~> 2.5) + thor (~> 1.0, >= 1.2.2) + zeitwerk (~> 2.6) rainbow (3.1.1) - rake (13.0.6) - regexp_parser (2.8.1) - reline (0.3.3) + rake (13.1.0) + rdoc (6.6.2) + psych (>= 4.0.0) + regexp_parser (2.9.0) + reline (0.4.2) io-console (~> 0.5) - rexml (3.2.5) - rubocop (1.54.2) + rexml (3.2.6) + rubocop (1.60.2) json (~> 2.3) language_server-protocol (>= 3.17.0) parallel (~> 1.10) - parser (>= 3.2.2.3) + parser (>= 3.3.0.2) rainbow (>= 2.2.2, < 4.0) regexp_parser (>= 1.8, < 3.0) rexml (>= 3.2.5, < 4.0) - rubocop-ast (>= 1.28.0, < 2.0) + rubocop-ast (>= 1.30.0, < 2.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 2.4.0, < 3.0) - rubocop-ast (1.29.0) + rubocop-ast (1.30.0) parser (>= 3.2.1.0) rubocop-shopify (2.14.0) rubocop (~> 1.51) ruby-progressbar (1.13.0) ruby2_keywords (0.0.5) rubyzip (2.3.2) - selenium-webdriver (4.10.0) + selenium-webdriver (4.17.0) + base64 (~> 0.2) rexml (~> 3.2, >= 3.2.5) rubyzip (>= 1.2.2, < 3.0) websocket (~> 1.0) @@ -226,29 +251,26 @@ GEM actionpack (>= 5.2) activesupport (>= 5.2) sprockets (>= 3.0.0) - sqlite3 (1.6.3) + sqlite3 (1.7.1) mini_portile2 (~> 2.8.0) - sqlite3 (1.6.3-arm64-darwin) - sqlite3 (1.6.3-x86_64-darwin) - sqlite3 (1.6.3-x86_64-linux) - thor (1.2.2) - timeout (0.4.0) + sqlite3 (1.7.1-arm64-darwin) + sqlite3 (1.7.1-x86_64-darwin) + sqlite3 (1.7.1-x86_64-linux) + stringio (3.1.0) + thor (1.3.0) + timeout (0.4.1) tzinfo (2.0.6) concurrent-ruby (~> 1.0) - unicode-display_width (2.4.2) - uri (0.12.2) - webdrivers (5.2.0) - nokogiri (~> 1.6) - rubyzip (>= 1.3.0) - selenium-webdriver (~> 4.0) - websocket (1.2.9) - websocket-driver (0.7.5) + unicode-display_width (2.5.0) + webrick (1.8.1) + websocket (1.2.10) + websocket-driver (0.7.6) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) xpath (3.2.0) nokogiri (~> 1.8) yard (0.9.34) - zeitwerk (2.6.8) + zeitwerk (2.6.12) PLATFORMS arm64-darwin @@ -262,16 +284,13 @@ DEPENDENCIES debug maintenance_tasks! mocha - net-http - net-smtp puma rails rubocop rubocop-shopify - selenium-webdriver (< 4.10.1) + selenium-webdriver sprockets-rails sqlite3 - webdrivers yard BUNDLED WITH diff --git a/README.md b/README.md index 66c122cd1..8312e943e 100644 --- a/README.md +++ b/README.md @@ -1,9 +1,64 @@ -# MaintenanceTasks +# Maintenance Tasks A Rails engine for queuing and managing maintenance tasks. +By ”maintenance task”, this project means a data migration, i.e. code that +changes data in the database, often to support schema migrations. For example, +in order to introduce a new `NOT NULL` column, it has to be added as nullable +first, backfilled with values, before finally being changed to `NOT NULL`. This +engine helps with the second part of this process, backfilling. + +Maintenance tasks are collection-based tasks, usually using Active Record, that +update the data in your database. They can be paused or interrupted. Maintenance +tasks can operate [in batches](#processing-batch-collections) and use +[throttling](#throttling) to control the load on your database. + +Maintenance tasks aren't meant to happen on a regular basis. They're used as +needed, or as one-offs. Normally maintenance tasks are ephemeral, so they are +used briefly and then deleted. + +The Rails engine has a web-based UI for listing maintenance tasks, seeing their +status, and starting, pausing and restarting them. + [![Link to demo video](static/demo.png)](https://www.youtube.com/watch?v=BTuvTQxlFzs) +## Should I Use Maintenance Tasks? + +Maintenance tasks have a limited, specific job UI. While the engine can be used +to provide a user interface for other data changes, such as data changes for +support requests, we recommend you use regular application code for those use +cases instead. These inevitably require more flexibility than this engine will +be able to provide. + +If your task shouldn't run as an Active Job, it probably isn't a good match for +this gem. If your task doesn't need to run in the background, consider a runner +script instead. If your task doesn't need to be interruptible, consider a normal +Active Job. + +Maintenance tasks can be interrupted between iterations. If your task [isn't +collection-based](#tasks-that-dont-need-a-collection) (no CSV file or database +table) or has very large batches, it will get limited benefit from throttling +(pausing between iterations) or interrupting. This might be fine, or the added +complexity of maintenance Tasks over normal Active Jobs may not be worthwhile. + +If your task updates your database schema instead of data, use a migration +instead of a maintenance task. + +If your task happens regularly, consider Active Jobs with a scheduler or cron, +[job-iteration jobs](https://github.com/shopify/job-iteration) and/or [custom +rails_admin UIs][rails-admin-engines] instead of the Maintenance Tasks gem. +Maintenance tasks should be ephemeral, to suit their intentionally limited UI. +They should not repeat. + +To create seed data for a new application, use the provided Rails `db/seeds.rb` +file instead. + +If your application can't handle a half-completed migration, maintenance tasks +are probably the wrong tool. Remember that maintenance tasks are intentionally +pausable and can be cancelled halfway. + +[rails-admin-engines]: https://www.ruby-toolbox.com/categories/rails_admin_interfaces + ## Installation To install the gem and run the install generator, execute: @@ -45,7 +100,8 @@ constants](https://guides.rubyonrails.org/autoloading_and_reloading_constants.ht The typical Maintenance Tasks workflow is as follows: -1. [Generate a class describing the Task](#creating-a-task) and the work to be done. +1. [Generate a class describing the Task](#creating-a-task) and the work to be + done. 2. Run the Task - either by [using the included web UI](#running-a-task-from-the-web-ui), - or by [using the command line](#running-a-task-from-the-command-line), @@ -137,9 +193,9 @@ title,content My Title,Hello World! ``` -The files uploaded to your Active Storage service provider will be renamed -to include an ISO 8601 timestamp and the Task name in snake case format. -The CSV is expected to have a trailing newline at the end of the file. +The files uploaded to your Active Storage service provider will be renamed to +include an ISO 8601 timestamp and the Task name in snake case format. The CSV is +expected to have a trailing newline at the end of the file. #### Batch CSV Tasks @@ -168,6 +224,9 @@ Note that `#count` is calculated automatically based on the number of batches in your collection, and your Task’s progress will be displayed in terms of batches (not the total number of rows in your CSV). +Non-batched CSV tasks will have an effective batch size of 1, which can reduce +the efficiency of your database operations. + ### Processing Batch Collections The Maintenance Tasks gem supports processing Active Records in batches. This @@ -213,8 +272,8 @@ inside `#process`. ### Tasks that don’t need a Collection Sometimes, you might want to run a Task that performs a single operation, such -as enqueuing another background job or hitting an external API. The gem supports -collection-less tasks. +as enqueuing another background job or querying an external API. The gem +supports collection-less tasks. Generate a collection-less Task by running: @@ -240,12 +299,43 @@ module Maintenance end ``` +### Tasks with Custom Enumerators + +If you have a special use case requiring iteration over an unsupported +collection type, such as external resources fetched from some API, you can +implement the `enumerator_builder(cursor:)` method in your task. + +This method should return an `Enumerator`, yielding pairs of +`[item, cursor]`. Maintenance Tasks takes care of persisting the current +cursor position and will provide it as the `cursor` argument if your task is +interrupted or resumed. The `cursor` is stored as a `String`, so your custom +enumerator should handle serializing/deserializing the value if required. + +```ruby +# app/tasks/maintenance/custom_enumerator_task.rb + +module Maintenance + class CustomEnumeratorTask < MaintenanceTasks::Task + def enumerator_builder(cursor:) + after_id = cursor&.to_i + PostAPI.index(after_id: after_id).map { |post| [post, post.id] }.to_enum + end + + def process(post) + Post.create!(post) + end + end +end +``` + ### Throttling -Maintenance Tasks often modify a lot of data and can be taxing on your database. +Maintenance tasks often modify a lot of data and can be taxing on your database. The gem provides a throttling mechanism that can be used to throttle a Task when -a given condition is met. If a Task is throttled, it will be interrupted and -retried after a backoff period has passed. The default backoff is 30 seconds. +a given condition is met. If a Task is throttled (the throttle block returns +true), it will be interrupted and retried after a backoff period has passed. The +default backoff is 30 seconds. + Specify the throttle condition as a block: ```ruby @@ -277,7 +367,7 @@ Tasks can define multiple throttle conditions. Throttle conditions are inherited by descendants, and new conditions will be appended without impacting existing conditions. -The backoff can also be specified as a Proc: +The backoff can also be specified as a Proc that receives no arguments: ```ruby # app/tasks/maintenance/update_posts_throttled_task.rb @@ -376,10 +466,9 @@ module Maintenance end ``` -Note: The `after_error` callback is guaranteed to complete, -so any exceptions raised in your callback code are ignored. -If your `after_error` callback code can raise an exception, -you’ll need to rescue it and handle it appropriately +Note: The `after_error` callback is guaranteed to complete, so any exceptions +raised in your callback code are ignored. If your `after_error` callback code +can raise an exception, you’ll need to rescue it and handle it appropriately within the callback. ```ruby @@ -388,7 +477,7 @@ module Maintenance after_error :dangerous_notify def dangerous_notify - # This error is rescued in favour of the original error causing the error flow. + # This error is rescued and ignored in favour of the original error causing the error flow. raise NotDeliveredError end @@ -397,9 +486,8 @@ module Maintenance end ``` -If any of the other callbacks cause an exception, -it will be handled by the error handler, -and will cause the task to stop running. +If any of the other callbacks cause an exception, it will be handled by the +error handler, and will cause the task to stop running. Callback behaviour can be shared across all tasks using an initializer. @@ -418,21 +506,21 @@ end ### Considerations when writing Tasks -MaintenanceTasks relies on the queue adapter configured for your application to +Maintenance Tasks relies on the queue adapter configured for your application to run the job which is processing your Task. The guidelines for writing Task may depend on the queue adapter but in general, you should follow these rules: * Duration of `Task#process`: processing a single element of the collection should take less than 25 seconds, or the duration set as a timeout for Sidekiq - or the queue adapter configured in your application. It allows the Task to be - safely interrupted and resumed. + or the queue adapter configured in your application. Short batches allow the + Task to be safely interrupted and resumed. * Idempotency of `Task#process`: it should be safe to run `process` multiple times for the same element of the collection. Read more in [this Sidekiq best practice][sidekiq-idempotent]. It’s important if the Task errors and you run - it again, because the same element that errored the Task may well be processed - again. It especially matters in the situation described above, when the - iteration duration exceeds the timeout: if the job is re-enqueued, multiple - elements may be processed again. + it again, because the same element that caused the Task to give an error may + well be processed again. It especially matters in the situation described + above, when the iteration duration exceeds the timeout: if the job is + re-enqueued, multiple elements may be processed again. [sidekiq-idempotent]: https://github.com/mperham/sidekiq/wiki/Best-Practices#2-make-your-job-idempotent-and-transactional @@ -441,14 +529,14 @@ depend on the queue adapter but in general, you should follow these rules: When the Task runs or resumes, the Runner enqueues a job, which processes the Task. That job will instantiate a Task object which will live for the duration of the job. The first time the job runs, it will call `count`. Every time a job -runs, it will call `collection` on the Task object, and then `process` -for each item in the collection, until the job stops. The job stops when either the +runs, it will call `collection` on the Task object, and then `process` for each +item in the collection, until the job stops. The job stops when either the collection is finished processing or after the maximum job runtime has expired. This means memoization can be misleading within `process`, since the memoized values will be available for subsequent calls to `process` within the same job. -Still, memoization can be used for throttling or reporting, and you can use [Task -callbacks](#using-task-callbacks) to persist or log a report for example. +Still, memoization can be used for throttling or reporting, and you can use +[Task callbacks](#using-task-callbacks) to persist or log a report for example. ### Writing tests for a Task @@ -526,7 +614,7 @@ module Maintenance test "#process performs a task iteration" do assert_difference -> { Post.first.content } do - task.process(Post.first) + @task.process(Post.first) end end end @@ -638,7 +726,7 @@ tweaked in an initializer if necessary. [max-job-runtime]: https://github.com/Shopify/job-iteration/blob/-/guides/best-practices.md#max-job-runtime Running tasks will also be interrupted and re-enqueued when needed. For example -[when Sidekiq workers shuts down for a deploy][sidekiq-deploy]: +[when Sidekiq workers shut down for a deploy][sidekiq-deploy]: [sidekiq-deploy]: https://github.com/mperham/sidekiq/wiki/Deployment @@ -651,19 +739,24 @@ Running tasks will also be interrupted and re-enqueued when needed. For example When Sidekiq is stopping, it will give workers 25 seconds to finish before forcefully terminating them (this is the default but can be configured with the `--timeout` option). Before the worker threads are terminated, Sidekiq will try -to re-enqueue the job so your Task will be resumed. However, the position in -the collection won’t be persisted so at least one iteration may run again. +to re-enqueue the job so your Task will be resumed. However, the position in the +collection won’t be persisted so at least one iteration may run again. + +Job queues other than Sidekiq may handle this in different ways. #### Help! My Task is stuck -Finally, if the queue adapter configured for your application doesn’t have this -property, or if Sidekiq crashes, is forcefully terminated, or is unable to -re-enqueue the jobs that were in progress, the Task may be in a seemingly stuck -situation where it appears to be running but is not. In that situation, pausing -or cancelling it will not result in the Task being paused or cancelled, as the -Task will get stuck in a state of `pausing` or `cancelling`. As a work-around, -if a Task is `cancelling` for more than 5 minutes, you will be able to cancel it -for good, which will just mark it as cancelled, allowing you to run it again. +If the queue adapter configured for your application doesn’t have this property, +or if Sidekiq crashes, is forcefully terminated, or is unable to re-enqueue the +jobs that were in progress, the Task may be in a seemingly stuck situation where +it appears to be running but is not. In that situation, pausing or cancelling it +will not result in the Task being paused or cancelled, as the Task will get +stuck in a state of `pausing` or `cancelling`. As a work-around, if a Task is +`cancelling` for more than 5 minutes, you can cancel it again. It will then be +marked as fully cancelled, allowing you to run it again. + +If you are stuck in `pausing` and wish to preserve your tasks's position +(instead of cancelling and rerunning), you may click "Force pause". ### Configuring the gem @@ -724,9 +817,10 @@ If no value is specified, it will default to `Maintenance`. #### Organizing tasks using namespaces -Tasks may be nested arbitrarily deeply under `app/tasks/maintenance`, for example given a -task file `app/tasks/maintenance/team_name/service_name/update_posts_task.rb` we -can define the task as: +Tasks may be nested arbitrarily deeply under `app/tasks/maintenance`, for +example given a task file +`app/tasks/maintenance/team_name/service_name/update_posts_task.rb` we can +define the task as: ```ruby module Maintenance @@ -815,8 +909,8 @@ default. #### Customizing the backtrace cleaner `MaintenanceTasks.backtrace_cleaner` can be configured to specify a backtrace -cleaner to use when a Task errors and the backtrace is cleaned and persisted. -An `ActiveSupport::BacktraceCleaner` should be used. +cleaner to use when a Task errors and the backtrace is cleaned and persisted. An +`ActiveSupport::BacktraceCleaner` should be used. ```ruby # config/initializers/maintenance_tasks.rb @@ -832,8 +926,8 @@ clean backtraces. #### Customizing the parent controller for the web UI -`MaintenanceTasks.parent_controller` can be configured to specify a controller class for all of the web UI engine's -controllers to inherit from. +`MaintenanceTasks.parent_controller` can be configured to specify a controller +class for all of the web UI engine's controllers to inherit from. This allows applications with common logic in their `ApplicationController` (or any other controller) to optionally configure the web UI to inherit that logic @@ -858,6 +952,29 @@ controller class which **must inherit** from `ActionController::Base`. If no value is specified, it will default to `"ActionController::Base"`. +#### Configure time after which the task will be considered stuck + +To specify a time duration after which a task is considered stuck if it has not been updated, +you can configure `MaintenanceTasks.stuck_task_duration`. This duration should account for +job infrastructure events that may prevent the maintenance tasks job from being executed and cancelling the task. + +The value for `MaintenanceTasks.stuck_task_duration` must be an `ActiveSupport::Duration`. +If no value is specified, it will default to 5 minutes. + +### Metadata + +`MaintenanceTasks.metadata` can be configured to specify a proc from which to +get extra information about the run. Since this proc will be ran in the context +of the `MaintenanceTasks.parent_controller`, it can be used to keep the id or +email of the user who performed the maintenance task. + +```ruby +# config/initializers/maintenance_tasks.rb +MaintenanceTasks.metadata = ->() do + { user_email: current_user.email } +end +``` + ## Upgrading Use bundler to check for and upgrade to newer versions. After installing a new @@ -869,7 +986,7 @@ bin/rails generate maintenance_tasks:install This ensures that new migrations are installed and run as well. -**What if I’ve deleted my previous Maintenance Task migrations?** +### What if I’ve deleted my previous Maintenance Task migrations? The install command will attempt to reinstall these old migrations and migrating the database will cause problems. Use `bin/rails diff --git a/app/controllers/maintenance_tasks/runs_controller.rb b/app/controllers/maintenance_tasks/runs_controller.rb index 2243acecd..3305b950c 100644 --- a/app/controllers/maintenance_tasks/runs_controller.rb +++ b/app/controllers/maintenance_tasks/runs_controller.rb @@ -13,7 +13,8 @@ def create(&block) task = Runner.run( name: params.fetch(:task_id), csv_file: params[:csv_file], - arguments: params.fetch(:task_arguments, {}).permit!.to_h, + arguments: params.fetch(:task, {}).permit!.to_h, + metadata: instance_exec(&MaintenanceTasks.metadata || -> {}), &block ) redirect_to(task_path(task)) @@ -28,7 +29,7 @@ def create(&block) # Updates a Run status to paused. def pause - @run.pausing! + @run.pause redirect_to(task_path(@run.task_name)) rescue ActiveRecord::RecordInvalid => error redirect_to(task_path(@run.task_name), alert: error.message) diff --git a/app/helpers/maintenance_tasks/tasks_helper.rb b/app/helpers/maintenance_tasks/tasks_helper.rb index 3d250d542..e9ed5fcc6 100644 --- a/app/helpers/maintenance_tasks/tasks_helper.rb +++ b/app/helpers/maintenance_tasks/tasks_helper.rb @@ -101,8 +101,17 @@ def csv_file_download_path(run) ) end - # Return the appropriate field tag for the parameter + # Return the appropriate field tag for the parameter, based on its type. + # If the parameter has a `validates_inclusion_of` validator, return a dropdown list of options instead. def parameter_field(form_builder, parameter_name) + inclusion_validator = form_builder.object.class.validators_on(parameter_name).find do |validator| + validator.kind == :inclusion + end + + return form_builder.select( + parameter_name, inclusion_validator.options[:in], prompt: "Select a value" + ) if inclusion_validator + case form_builder.object.class.attribute_types[parameter_name] when ActiveModel::Type::Integer form_builder.number_field(parameter_name) diff --git a/app/jobs/concerns/maintenance_tasks/task_job_concern.rb b/app/jobs/concerns/maintenance_tasks/task_job_concern.rb index c5b6b8859..a39649b29 100644 --- a/app/jobs/concerns/maintenance_tasks/task_job_concern.rb +++ b/app/jobs/concerns/maintenance_tasks/task_job_concern.rb @@ -32,50 +32,7 @@ def retry_on(*, **) def build_enumerator(_run, cursor:) cursor ||= @run.cursor - collection = @task.collection - @enumerator = nil - - @collection_enum = case collection - when :no_collection - enumerator_builder.build_once_enumerator(cursor: nil) - when ActiveRecord::Relation - enumerator_builder.active_record_on_records( - collection, - cursor: cursor, - **{ columns: @task.class.cursor_columns }.compact, - ) - when ActiveRecord::Batches::BatchEnumerator - if collection.start || collection.finish - raise ArgumentError, <<~MSG.squish - #{@task.class.name}#collection cannot support - a batch enumerator with the "start" or "finish" options. - MSG - end - - # For now, only support automatic count based on the enumerator for - # batches - enumerator_builder.active_record_on_batch_relations( - collection.relation, - cursor: cursor, - batch_size: collection.batch_size, - **{ columns: @task.class.cursor_columns }.compact, - ) - when Array - enumerator_builder.build_array_enumerator(collection, cursor: cursor&.to_i) - when BatchCsvCollectionBuilder::BatchCsv - JobIteration::CsvEnumerator.new(collection.csv).batches( - batch_size: collection.batch_size, - cursor: cursor&.to_i, - ) - when CSV - JobIteration::CsvEnumerator.new(collection).rows(cursor: cursor&.to_i) - else - raise ArgumentError, <<~MSG.squish - #{@task.class.name}#collection must be either an - Active Record Relation, ActiveRecord::Batches::BatchEnumerator, - Array, or CSV. - MSG - end + @collection_enum = @task.enumerator_builder(cursor: cursor) throttle_enumerator(@collection_enum) end @@ -170,6 +127,7 @@ def on_error(error) @ticker.persist if defined?(@ticker) if defined?(@run) + @run.cursor = cursor_position @run.persist_error(error) task_context = { diff --git a/app/models/maintenance_tasks/run.rb b/app/models/maintenance_tasks/run.rb index 647e90cce..414eb16f7 100644 --- a/app/models/maintenance_tasks/run.rb +++ b/app/models/maintenance_tasks/run.rb @@ -32,13 +32,10 @@ class Run < ApplicationRecord :cancelled, ] COMPLETED_STATUSES = [:succeeded, :errored, :cancelled] - STUCK_TASK_TIMEOUT = 5.minutes enum status: STATUSES.to_h { |status| [status, status.to_s] } - validates :task_name, on: :create, inclusion: { - in: ->(_) { Task.available_tasks.map(&:to_s) }, - } + validate :task_name_belongs_to_a_valid_task, on: :create validate :csv_attachment_presence, on: :create validate :csv_content_type, on: :create validate :validate_task_arguments, on: :create @@ -48,9 +45,11 @@ class Run < ApplicationRecord if Rails.gem_version >= Gem::Version.new("7.1.alpha") serialize :backtrace, coder: YAML serialize :arguments, coder: JSON + serialize :metadata, coder: JSON else serialize :backtrace serialize :arguments, JSON + serialize :metadata, JSON end scope :active, -> { where(status: ACTIVE_STATUSES) } @@ -320,21 +319,38 @@ def cancel # Marks a Run as pausing. # + # If the Run has been stuck on pausing for more than 5 minutes, it forces + # the transition to paused. The ended_at timestamp will be updated. + # # Rescues and retries status transition if an ActiveRecord::StaleObjectError # is encountered. - def pausing! - super + def pause + if stuck? + self.status = :paused + persist_transition + else + pausing! + end rescue ActiveRecord::StaleObjectError reload_status retry end # Returns whether a Run is stuck, which is defined as having a status of - # cancelling, and not having been updated in the last 5 minutes. + # cancelling or pausing, and not having been updated in the last 5 minutes. # # @return [Boolean] whether the Run is stuck. def stuck? - cancelling? && updated_at <= STUCK_TASK_TIMEOUT.ago + (cancelling? || pausing?) && updated_at <= MaintenanceTasks.stuck_task_duration.ago + end + + # Performs validation on the task_name attribute. + # A Run must be associated with a valid Task to be valid. + # In order to confirm that, the Task is looked up by name. + def task_name_belongs_to_a_valid_task + Task.named(task_name) + rescue Task::NotFoundError + errors.add(:task_name, "must be the name of an existing Task.") end # Performs validation on the presence of a :csv_file attachment. diff --git a/app/models/maintenance_tasks/runner.rb b/app/models/maintenance_tasks/runner.rb index 4b73ef91f..82f3e9e9b 100644 --- a/app/models/maintenance_tasks/runner.rb +++ b/app/models/maintenance_tasks/runner.rb @@ -39,8 +39,8 @@ def initialize(run) # creating the Run. # @raise [ActiveRecord::ValueTooLong] if the creation of the Run fails due # to a value being too long for the column type. - def run(name:, csv_file: nil, arguments: {}, run_model: Run) - run = run_model.new(task_name: name, arguments: arguments) + def run(name:, csv_file: nil, arguments: {}, run_model: Run, metadata: nil) + run = run_model.new(task_name: name, arguments: arguments, metadata: metadata) if csv_file run.csv_file.attach(csv_file) run.csv_file.filename = filename(name) diff --git a/app/models/maintenance_tasks/task.rb b/app/models/maintenance_tasks/task.rb index 31dfacd31..f2cda3b1a 100644 --- a/app/models/maintenance_tasks/task.rb +++ b/app/models/maintenance_tasks/task.rb @@ -44,15 +44,27 @@ def named(name) task end - # Returns a list of concrete classes that inherit from the Task - # superclass. + # Loads and returns a list of concrete classes that inherit + # from the Task superclass. # # @return [Array] the list of classes. - def available_tasks + def load_all load_constants descendants end + # Loads and returns a list of concrete classes that inherit + # from the Task superclass. + # + # @return [Array] the list of classes. + def available_tasks + warn(<<~MSG.squish, category: :deprecated) + MaintenanceTasks::Task.available_tasks is deprecated and will be + removed from maintenance-tasks 3.0.0. Use .load_all instead. + MSG + load_all + end + # Make this Task a task that handles CSV. # # @param in_batches [Integer] optionally, supply a batch size if the CSV @@ -178,13 +190,7 @@ def load_constants namespace = MaintenanceTasks.tasks_module.safe_constantize return unless namespace - load_const = lambda do |root| - root.constants.each do |name| - object = root.const_get(name) - load_const.call(object) if object.instance_of?(Module) - end - end - load_const.call(namespace) + Rails.autoloaders.main.eager_load_namespace(namespace) end end @@ -244,5 +250,60 @@ def process(_item) def count self.class.collection_builder_strategy.count(self) end + + # Default enumeration builder. You may override this method to return any + # Enumerator yielding pairs of `[item, item_cursor]`. + # + # @param cursor [String, nil] cursor position to resume from, or nil on + # initial call. + # + # @return [Enumerator] + def enumerator_builder(cursor:) + collection = self.collection + + job_iteration_builder = JobIteration::EnumeratorBuilder.new(nil) + + case collection + when :no_collection + job_iteration_builder.build_once_enumerator(cursor: nil) + when ActiveRecord::Relation + job_iteration_builder.active_record_on_records( + collection, + cursor: cursor, + **{ columns: self.class.cursor_columns }.compact, + ) + when ActiveRecord::Batches::BatchEnumerator + if collection.start || collection.finish + raise ArgumentError, <<~MSG.squish + #{self.class.name}#collection cannot support + a batch enumerator with the "start" or "finish" options. + MSG + end + + # For now, only support automatic count based on the enumerator for + # batches + job_iteration_builder.active_record_on_batch_relations( + collection.relation, + cursor: cursor, + batch_size: collection.batch_size, + **{ columns: self.class.cursor_columns }.compact, + ) + when Array + job_iteration_builder.build_array_enumerator(collection, cursor: cursor&.to_i) + when BatchCsvCollectionBuilder::BatchCsv + JobIteration::CsvEnumerator.new(collection.csv).batches( + batch_size: collection.batch_size, + cursor: cursor&.to_i, + ) + when CSV + JobIteration::CsvEnumerator.new(collection).rows(cursor: cursor&.to_i) + else + raise ArgumentError, <<~MSG.squish + #{self.class.name}#collection must be either an + Active Record Relation, ActiveRecord::Batches::BatchEnumerator, + Array, or CSV. + MSG + end + end end end diff --git a/app/models/maintenance_tasks/task_data_index.rb b/app/models/maintenance_tasks/task_data_index.rb index 7ad5c9cb4..b31d81d56 100644 --- a/app/models/maintenance_tasks/task_data_index.rb +++ b/app/models/maintenance_tasks/task_data_index.rb @@ -25,7 +25,7 @@ class << self def available_tasks tasks = [] - task_names = Task.available_tasks.map(&:name) + task_names = Task.load_all.map(&:name) active_runs = Run.with_attached_csv.active.where(task_name: task_names) active_runs.each do |run| diff --git a/app/validators/maintenance_tasks/run_status_validator.rb b/app/validators/maintenance_tasks/run_status_validator.rb index f56e95386..c4a30637b 100644 --- a/app/validators/maintenance_tasks/run_status_validator.rb +++ b/app/validators/maintenance_tasks/run_status_validator.rb @@ -60,6 +60,9 @@ class RunStatusValidator < ActiveModel::Validator # interrupted -> errored occurs when the task is deleted while it is # interrupted. "interrupted" => ["running", "pausing", "cancelling", "errored"], + # errored -> enqueued occurs when the task is retried after encounting an + # error. + "errored" => ["enqueued"], } # Validate whether a transition from one Run status diff --git a/app/views/maintenance_tasks/runs/_arguments.html.erb b/app/views/maintenance_tasks/runs/_arguments.html.erb index ef9003335..e7f809bdb 100644 --- a/app/views/maintenance_tasks/runs/_arguments.html.erb +++ b/app/views/maintenance_tasks/runs/_arguments.html.erb @@ -1,22 +1,4 @@ -<% if run.arguments.present? %> -
-
Arguments:
- - - <% run.arguments.transform_values(&:to_s).each do |key, value| %> - - - - - <% end %> - -
<%= key %> - <% next if value.empty? %> - <% if value.include?("\n") %> -
<%= value %>
- <% else %> - <%= value %> - <% end %> -
-
+<% if arguments.present? %> +
Arguments:
+ <%= render "maintenance_tasks/runs/serializable", serializable: arguments %> <% end %> diff --git a/app/views/maintenance_tasks/runs/_metadata.html.erb b/app/views/maintenance_tasks/runs/_metadata.html.erb new file mode 100644 index 000000000..8e5a98924 --- /dev/null +++ b/app/views/maintenance_tasks/runs/_metadata.html.erb @@ -0,0 +1,4 @@ +<% if metadata.present? %> +
Metadata:
+ <%= render "maintenance_tasks/runs/serializable", serializable: metadata %> +<% end %> diff --git a/app/views/maintenance_tasks/runs/_run.html.erb b/app/views/maintenance_tasks/runs/_run.html.erb index 9a8e4c4a1..381aa0c99 100644 --- a/app/views/maintenance_tasks/runs/_run.html.erb +++ b/app/views/maintenance_tasks/runs/_run.html.erb @@ -2,6 +2,7 @@
<%= time_tag run.created_at, title: run.created_at.utc.iso8601 %> <%= status_tag run.status %> + #<%= run.id %>
<%= progress run %> @@ -16,12 +17,16 @@ <%= render "maintenance_tasks/runs/csv", run: run %> <%= tag.hr if run.csv_file.present? && run.arguments.present? %> - <%= render "maintenance_tasks/runs/arguments", run: run %> + <%= render "maintenance_tasks/runs/arguments", arguments: run.arguments %> + <%= tag.hr if run.csv_file.present? || run.arguments.present? && run.metadata.present? %> + <%= render "maintenance_tasks/runs/metadata", metadata: run.metadata %>
<% if run.paused? %> <%= button_to 'Resume', resume_task_run_path(@task, run), method: :put, class: 'button is-primary', disabled: @task.deleted? %> <%= button_to 'Cancel', cancel_task_run_path(@task, run), method: :put, class: 'button is-danger' %> + <% elsif run.errored? %> + <%= button_to 'Resume', resume_task_run_path(@task, run), method: :put, class: 'button is-primary', disabled: @task.deleted? %> <% elsif run.cancelling? %> <% if run.stuck? %> <%= button_to 'Cancel', cancel_task_run_path(@task, run), method: :put, class: 'button is-danger', disabled: @task.deleted? %> @@ -29,6 +34,9 @@ <% elsif run.pausing? %> <%= button_to 'Pausing', pause_task_run_path(@task, run), method: :put, class: 'button is-warning', disabled: true %> <%= button_to 'Cancel', cancel_task_run_path(@task, run), method: :put, class: 'button is-danger' %> + <% if run.stuck? %> + <%= button_to 'Force pause', pause_task_run_path(@task, run), method: :put, class: 'button is-danger', disabled: @task.deleted? %> + <% end %> <% elsif run.active? %> <%= button_to 'Pause', pause_task_run_path(@task, run), method: :put, class: 'button is-warning', disabled: @task.deleted? %> <%= button_to 'Cancel', cancel_task_run_path(@task, run), method: :put, class: 'button is-danger' %> diff --git a/app/views/maintenance_tasks/runs/_serializable.html.erb b/app/views/maintenance_tasks/runs/_serializable.html.erb new file mode 100644 index 000000000..6c79e3310 --- /dev/null +++ b/app/views/maintenance_tasks/runs/_serializable.html.erb @@ -0,0 +1,26 @@ +<% if serializable.present? %> + <% case serializable %> + <% when Hash %> +
+ + + <% serializable.transform_values(&:to_s).each do |key, value| %> + + + + + <% end %> + +
<%= key %> + <% next if value.empty? %> + <% if value.include?("\n") %> +
<%= value %>
+ <% else %> + <%= value %> + <% end %> +
+
+ <% else %> + <%= serializable.inspect %> + <% end %> +<% end %> diff --git a/app/views/maintenance_tasks/tasks/_task.html.erb b/app/views/maintenance_tasks/tasks/_task.html.erb index 614aebcbc..1fff619b8 100644 --- a/app/views/maintenance_tasks/tasks/_task.html.erb +++ b/app/views/maintenance_tasks/tasks/_task.html.erb @@ -21,6 +21,8 @@ <%= render "maintenance_tasks/runs/csv", run: run %> <%= tag.hr if run.csv_file.present? && run.arguments.present? %> - <%= render "maintenance_tasks/runs/arguments", run: run %> + <%= render "maintenance_tasks/runs/arguments", arguments: run.arguments %> + <%= tag.hr if run.csv_file.present? || run.arguments.present? && run.metadata.present? %> + <%= render "maintenance_tasks/runs/metadata", metadata: run.metadata %> <% end %>
diff --git a/app/views/maintenance_tasks/tasks/show.html.erb b/app/views/maintenance_tasks/tasks/show.html.erb index 3fbac1b11..4304c53fb 100644 --- a/app/views/maintenance_tasks/tasks/show.html.erb +++ b/app/views/maintenance_tasks/tasks/show.html.erb @@ -15,7 +15,7 @@ <% parameter_names = @task.parameter_names %> <% if parameter_names.any? %>
- <%= form.fields_for :task_arguments, @task.new do |ff| %> + <%= fields_for :task, @task.new do |ff| %> <% parameter_names.each do |parameter_name| %>
<%= ff.label parameter_name, parameter_name, class: "label is-family-monospace" %> diff --git a/bin/setup b/bin/setup new file mode 100755 index 000000000..3b6facc33 --- /dev/null +++ b/bin/setup @@ -0,0 +1,6 @@ +#!/bin/sh + +set -ex + +bundle install +bin/rails db:setup diff --git a/db/migrate/20230622035229_add_metadata_to_runs.rb b/db/migrate/20230622035229_add_metadata_to_runs.rb new file mode 100644 index 000000000..c64f1299c --- /dev/null +++ b/db/migrate/20230622035229_add_metadata_to_runs.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddMetadataToRuns < ActiveRecord::Migration[6.0] + def change + add_column(:maintenance_tasks_runs, :metadata, :text) + end +end diff --git a/lib/generators/maintenance_tasks/templates/no_collection_task_test.rb.tt b/lib/generators/maintenance_tasks/templates/no_collection_task_test.rb.tt deleted file mode 100644 index 366cbb4cb..000000000 --- a/lib/generators/maintenance_tasks/templates/no_collection_task_test.rb.tt +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -module <%= tasks_module %> -<% module_namespacing do -%> - class <%= class_name %>TaskTest < ActiveSupport::TestCase - # test "#process performs a task iteration" do - # <%= tasks_module %>::<%= class_name %>Task.process - # end - end -<% end -%> -end diff --git a/lib/generators/maintenance_tasks/templates/task_spec.rb.tt b/lib/generators/maintenance_tasks/templates/task_spec.rb.tt index ffb36cfa1..755b8293c 100644 --- a/lib/generators/maintenance_tasks/templates/task_spec.rb.tt +++ b/lib/generators/maintenance_tasks/templates/task_spec.rb.tt @@ -6,10 +6,14 @@ module <%= tasks_module %> <% module_namespacing do -%> RSpec.describe <%= class_name %>Task do describe "#process" do + <%- if no_collection? -%> + subject(:process) { described_class.process } + <%- else -%> subject(:process) { described_class.process(element) } let(:element) { # Object to be processed in a single iteration of this task } + <%- end -%> pending "add some examples to (or delete) #{__FILE__}" end end diff --git a/lib/maintenance_tasks.rb b/lib/maintenance_tasks.rb index 0d34f2b0a..643a2ca28 100644 --- a/lib/maintenance_tasks.rb +++ b/lib/maintenance_tasks.rb @@ -82,4 +82,18 @@ module MaintenanceTasks # # @return [String] the name of the parent controller for web UI. mattr_accessor :parent_controller, default: "ActionController::Base" + + # @!attribute metadata + # @scope class + # The Proc to call from the controller to generate metadata that will be persisted on the Run. + # + # @return [Proc] generates a hash containing the metadata to be stored on the Run + mattr_accessor :metadata, default: nil + + # @!attribute stuck_task_duration + # @scope class + # The duration after which a task is considered stuck and can be force cancelled. + # + # @return [ActiveSupport::Duration] the threshold in seconds after which a task is considered stuck. + mattr_accessor :stuck_task_duration, default: 5.minutes end diff --git a/lib/maintenance_tasks/cli.rb b/lib/maintenance_tasks/cli.rb index ac5ba1ce3..a5e97b2b2 100644 --- a/lib/maintenance_tasks/cli.rb +++ b/lib/maintenance_tasks/cli.rb @@ -15,15 +15,12 @@ def exit_on_failure? end desc "perform [TASK NAME]", "Runs the given Maintenance Task" + long_desc <<~DESC + `maintenance_tasks perform` will run the Maintenance Task specified by + the [TASK NAME] argument. - long_desc <<-LONGDESC - `maintenance_tasks perform` will run the Maintenance Task specified by the - [TASK NAME] argument. - - Available Tasks: - - #{MaintenanceTasks::Task.available_tasks.join("\n\n")} - LONGDESC + Use `maintenance_tasks list` to get a list of all available tasks. + DESC # Specify the CSV file to process for CSV Tasks desc = "Supply a CSV file to be processed by a CSV Task, "\ @@ -50,6 +47,16 @@ def perform(name) say_status(:error, error.message, :red) end + desc "list", "Load and list all available tasks." + + # Command to list all available Tasks. + # + # It needs to use `Task.load_all` in order to load all the tasks available + # in the project before displaying them. + def list + say(Task.load_all.map(&:name).sort.join("\n")) + end + private def csv_file diff --git a/maintenance_tasks.gemspec b/maintenance_tasks.gemspec index a51338f88..a254c2130 100644 --- a/maintenance_tasks.gemspec +++ b/maintenance_tasks.gemspec @@ -2,13 +2,15 @@ Gem::Specification.new do |spec| spec.name = "maintenance_tasks" - spec.version = "2.1.1" + spec.version = "2.4.0" spec.author = "Shopify Engineering" spec.email = "gems@shopify.com" spec.homepage = "https://github.com/Shopify/maintenance_tasks" spec.summary = "A Rails engine for queuing and managing maintenance tasks" spec.license = "MIT" + spec.required_ruby_version = ">= 3.0" + spec.metadata = { "source_code_uri" => "https://github.com/Shopify/maintenance_tasks/tree/v#{spec.version}", @@ -22,6 +24,7 @@ Gem::Specification.new do |spec| spec.add_dependency("actionpack", ">= 6.0") spec.add_dependency("activejob", ">= 6.0") spec.add_dependency("activerecord", ">= 6.0") - spec.add_dependency("job-iteration", "~> 1.3.6") + spec.add_dependency("job-iteration", ">= 1.3.6") spec.add_dependency("railties", ">= 6.0") + spec.add_dependency("zeitwerk", ">= 2.6.2") end diff --git a/test/application_system_test_case.rb b/test/application_system_test_case.rb index 4441cc2f4..a90d46801 100644 --- a/test/application_system_test_case.rb +++ b/test/application_system_test_case.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "test_helper" -require "webdrivers/chromedriver" require "action_dispatch/system_testing/server" ActionDispatch::SystemTesting::Server.silence_puma = true @@ -14,6 +13,11 @@ if Rails::VERSION::MAJOR < 7 Selenium::WebDriver.logger.ignore(:browser_options) + if Rails::VERSION::MINOR < 1 + require "action_dispatch/system_testing/browser" + # prevent Rails from adding --headless at the end of the arguments, overriding --headless=new + ActionDispatch::SystemTesting::Browser.redefine_method(:options) { capabilities } + end elsif Rails.gem_version < Gem::Version.new("7.1") Selenium::WebDriver.logger.ignore(:capabilities) end @@ -23,14 +27,12 @@ class ApplicationSystemTestCase < ActionDispatch::SystemTestCase driven_by :selenium, using: :headless_chrome do |options| options.add_argument("--disable-dev-shm-usage") - options.add_preference( - :download, - default_directory: "test/dummy/tmp/downloads", - ) + options.add_argument("--headless=new") end setup do travel_to Time.zone.local(2020, 1, 9, 9, 41, 44) + page.driver.browser.download_path = "test/dummy/tmp/downloads" end teardown do diff --git a/test/dummy/app/tasks/maintenance/custom_enumerating_task.rb b/test/dummy/app/tasks/maintenance/custom_enumerating_task.rb new file mode 100644 index 000000000..0633efb75 --- /dev/null +++ b/test/dummy/app/tasks/maintenance/custom_enumerating_task.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Maintenance + class CustomEnumeratingTask < MaintenanceTasks::Task + def enumerator_builder(cursor:) + drop = cursor.nil? ? 0 : cursor.to_i + 1 + + [:a, :b, :c].lazy.with_index.drop(drop) + end + + def count + 3 + end + + def process(_) + end + end +end diff --git a/test/dummy/app/tasks/maintenance/error_task.rb b/test/dummy/app/tasks/maintenance/error_task.rb index c51203255..9674ed44d 100644 --- a/test/dummy/app/tasks/maintenance/error_task.rb +++ b/test/dummy/app/tasks/maintenance/error_task.rb @@ -3,11 +3,11 @@ module Maintenance class ErrorTask < MaintenanceTasks::Task def collection - [1, 2] + [1, 2, 3, 4, 5] end def process(input) - raise ArgumentError, "Something went wrong" if input == 2 + raise ArgumentError, "Something went wrong" if input == 3 end end end diff --git a/test/dummy/app/tasks/maintenance/params_task.rb b/test/dummy/app/tasks/maintenance/params_task.rb index 6cfdb43e0..34787400c 100644 --- a/test/dummy/app/tasks/maintenance/params_task.rb +++ b/test/dummy/app/tasks/maintenance/params_task.rb @@ -17,6 +17,9 @@ class ParamsTask < MaintenanceTasks::Task attribute :date_attr, :date attribute :time_attr, :time attribute :boolean_attr, :boolean + attribute :enum_attr, :integer + + validates_inclusion_of :enum_attr, in: [100, 200, 300], allow_nil: true class << self attr_accessor :fast_task diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index f686ae80e..4032acb9d 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_07_13_131925) do +ActiveRecord::Schema.define(version: 2023_06_22_035229) do create_table "active_storage_attachments", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -56,6 +56,7 @@ t.datetime "updated_at", precision: 6, null: false t.text "arguments" t.integer "lock_version", default: 0, null: false + t.text "metadata" t.index ["task_name", "status", "created_at"], name: "index_maintenance_tasks_runs", order: { created_at: :desc } end diff --git a/test/helpers/maintenance_tasks/tasks_helper_test.rb b/test/helpers/maintenance_tasks/tasks_helper_test.rb index d72e66c54..88d24723f 100644 --- a/test/helpers/maintenance_tasks/tasks_helper_test.rb +++ b/test/helpers/maintenance_tasks/tasks_helper_test.rb @@ -128,7 +128,7 @@ class TasksHelperTest < ActionView::TestCase # Time.zone_default Time.use_zone(now.utc_offset + 2.hour * sign) do # a timezone that is not the system timezone with_zone_default(Time.find_zone!(now.utc_offset + 1.hour * sign)) do # another non-system timezone - value = ActiveModel::Type::DateTime.new.cast("2023-06-01T12:00:00") # a local ISO8601 time (no timezone) + value = ActiveModel::Type::DateTime.new.cast(now.strftime("%FT%T")) # a local ISO8601 time (no timezone) refute_predicate(value, :utc?) assert_equal(now.utc_offset, value.utc_offset) # uses system time zone end diff --git a/test/jobs/maintenance_tasks/task_job_test.rb b/test/jobs/maintenance_tasks/task_job_test.rb index 6ae76a0b6..5090cdbf8 100644 --- a/test/jobs/maintenance_tasks/task_job_test.rb +++ b/test/jobs/maintenance_tasks/task_job_test.rb @@ -35,7 +35,7 @@ class TaskJobTest < ActiveJob::TestCase freeze_time TaskJob.perform_later(@run) @run.cancel - travel Run::STUCK_TASK_TIMEOUT + travel MaintenanceTasks.stuck_task_duration @run.cancel # force cancel the Run assert_predicate @run, :cancelled? Maintenance::TestTask.any_instance.expects(:process).never @@ -158,14 +158,15 @@ class TaskJobTest < ActiveJob::TestCase freeze_time run = Run.create!(task_name: "Maintenance::ErrorTask") - run.expects(:persist_error).with do |exception| - assert_kind_of ArgumentError, exception - assert_equal "Something went wrong", exception.message - expected = "app/tasks/maintenance/error_task.rb:10:in `process'" - assert_match expected, exception.backtrace.first - end - TaskJob.perform_now(run) + run.reload + + assert_equal "ArgumentError", run.error_class + assert_equal "Something went wrong", run.error_message + expected_backtrace = "app/tasks/maintenance/error_task.rb:10:in `process'" + assert_match expected_backtrace, run.backtrace.first + assert_equal Time.now, run.ended_at + assert_equal "1", run.cursor end test ".perform_now handles when the Task cannot be found" do @@ -350,7 +351,7 @@ class << self assert_equal(ArgumentError, handled_error.class) assert_equal("Maintenance::ErrorTask", handled_task_context[:task_name]) - assert_equal(2, handled_errored_element) + assert_equal(3, handled_errored_element) ensure MaintenanceTasks.error_handler = error_handler_before end @@ -366,7 +367,7 @@ class << self run.reload assert_predicate(run, :errored?) - assert_equal(1, run.tick_count) + assert_equal(2, run.tick_count) ensure MaintenanceTasks.error_handler = error_handler_before end @@ -653,5 +654,36 @@ class << self assert_equal 2, @run.reload.tick_total end + + test ".perform_now accepts custom enumerated tasks" do + run = Run.create!(task_name: "Maintenance::CustomEnumeratingTask") + + [:a, :b, :c].each do |item| + Maintenance::CustomEnumeratingTask.any_instance + .expects(:process).with(item).once + end + + TaskJob.perform_now(run) + end + + test ".perform_now handles cursors provided by custom enumerated tasks" do + run = Run.create!(task_name: "Maintenance::CustomEnumeratingTask") + + TaskJob.perform_now(run) + + assert_equal "2", run.reload.cursor + end + + test ".perform_now starts custom enumerated tasks from cursor position when job resumes" do + run = Run.create!(task_name: "Maintenance::CustomEnumeratingTask") + run.update!(cursor: "0") + + [:b, :c].each do |item| + Maintenance::CustomEnumeratingTask.any_instance + .expects(:process).with(item).once + end + + TaskJob.perform_now(run) + end end end diff --git a/test/lib/generators/maintenance_tasks/task_generator_test.rb b/test/lib/generators/maintenance_tasks/task_generator_test.rb index f51ef619f..2dd701eb6 100644 --- a/test/lib/generators/maintenance_tasks/task_generator_test.rb +++ b/test/lib/generators/maintenance_tasks/task_generator_test.rb @@ -25,10 +25,8 @@ def teardown end assert_file "test/tasks/maintenance/sleepy_task_test.rb" do |task_test| assert_match(/module Maintenance/, task_test) - assert_match( - /class SleepyTaskTest < ActiveSupport::TestCase/, - task_test, - ) + assert_match(/class SleepyTaskTest < ActiveSupport::TestCase/, task_test) + assert_match(/Maintenance::SleepyTask\.process\(element\)/, task_test) end end @@ -42,6 +40,7 @@ def teardown assert_file("spec/tasks/maintenance/sleepy_task_spec.rb") do |task_spec| assert_match(/module Maintenance/, task_spec) assert_match(/RSpec.describe SleepyTask/, task_spec) + assert_match(/subject\(:process\) \{ described_class.process\(element\) \}/, task_spec) end ensure generators_config.options[:rails][:test_framework] = old_test_framework @@ -91,6 +90,26 @@ def teardown end end + test "generator creates a collection-less task test if the --no-collection option is supplied" do + run_generator(["sleepy", "--no-collection"]) + assert_file("test/tasks/maintenance/sleepy_task_test.rb") do |task_test| + assert_match(/Maintenance::SleepyTask\.process$/, task_test) + end + end + + test "generator creates a collection-less task spec if the --no-collection option is supplied and the application is using RSpec" do + generators_config = Rails.application.config.generators + old_test_framework = generators_config.options[:rails][:test_framework] + generators_config.options[:rails][:test_framework] = :rspec + + run_generator(["sleepy", "--no-collection"]) + assert_file("spec/tasks/maintenance/sleepy_task_spec.rb") do |task_spec| + assert_match(/subject\(:process\) \{ described_class.process \}/, task_spec) + end + ensure + generators_config.options[:rails][:test_framework] = old_test_framework + end + test "generator raises if multiple collection options provided" do error = assert_raises do run_generator(["sleepy", "--csv", "--no-collection"]) diff --git a/test/lib/maintenance_tasks/cli_test.rb b/test/lib/maintenance_tasks/cli_test.rb index 795ebfd82..fc8c1199b 100644 --- a/test/lib/maintenance_tasks/cli_test.rb +++ b/test/lib/maintenance_tasks/cli_test.rb @@ -84,5 +84,18 @@ class CLITest < ActiveSupport::TestCase CLI.start(["perform", "MyParamsTask", "--arguments", "post_ids:1,2,3"]) end end + + test "`list` loads all tasks and displays them" do + Task.expects(:load_all).returns([stub(name: "Task1"), stub(name: "Task2")]) + + expected_output = <<~OUTPUT + Task1 + Task2 + OUTPUT + + assert_output(expected_output) do + CLI.start(["list"]) + end + end end end diff --git a/test/models/maintenance_tasks/run_test.rb b/test/models/maintenance_tasks/run_test.rb index fec1609f5..6904d01e6 100644 --- a/test/models/maintenance_tasks/run_test.rb +++ b/test/models/maintenance_tasks/run_test.rb @@ -527,18 +527,18 @@ class RunTest < ActiveSupport::TestCase ) refute_predicate run, :stuck? - travel Run::STUCK_TASK_TIMEOUT + travel MaintenanceTasks.stuck_task_duration assert_predicate run, :stuck? end test "#stuck? does not return true for other statuses" do freeze_time - Run.statuses.except("cancelling").each_key do |status| + Run.statuses.except("cancelling", "pausing").each_key do |status| run = Run.create!( task_name: "Maintenance::UpdatePostsTask", status: status, ) - travel Run::STUCK_TASK_TIMEOUT + travel MaintenanceTasks.stuck_task_duration refute_predicate run, :stuck? end end @@ -554,7 +554,7 @@ class RunTest < ActiveSupport::TestCase assert_predicate run, :cancelling? assert_nil run.ended_at - travel Run::STUCK_TASK_TIMEOUT + travel MaintenanceTasks.stuck_task_duration run.cancel assert_predicate run, :cancelled? assert_equal Time.now, run.ended_at @@ -624,17 +624,33 @@ class RunTest < ActiveSupport::TestCase assert_predicate run, :cancelling? end - test "#pausing! rescues and retries ActiveRecord::StaleObjectError" do + test "#pause rescues and retries ActiveRecord::StaleObjectError" do run = Run.create!(task_name: "Maintenance::UpdatePostsTask") Run.find(run.id).running! assert_nothing_raised do - run.pausing! + run.pause end assert_predicate run, :pausing? end + test "#pause transitions from pausing to paused if it has not been updated in more than 5 minutes" do + freeze_time + run = Run.create!( + task_name: "Maintenance::UpdatePostsTask", + status: :pausing, + ) + + run.pause + assert_predicate run, :pausing? + assert_nil run.ended_at + + travel MaintenanceTasks.stuck_task_duration + run.pause + assert_predicate run, :paused? + end + test "#persist_error rescues and retries ActiveRecord::StaleObjectError" do run = Run.create!( task_name: "Maintenance::ErrorTask", diff --git a/test/models/maintenance_tasks/runner_test.rb b/test/models/maintenance_tasks/runner_test.rb index 7bfd99252..a86eb6af0 100644 --- a/test/models/maintenance_tasks/runner_test.rb +++ b/test/models/maintenance_tasks/runner_test.rb @@ -61,7 +61,7 @@ class RunnerTest < ActiveSupport::TestCase end assert_equal( - "Validation failed: Task name is not included in the list", + "Validation failed: Task name must be the name of an existing Task.", error.message, ) end diff --git a/test/models/maintenance_tasks/task_data_index_test.rb b/test/models/maintenance_tasks/task_data_index_test.rb index c51357dc8..68f7ebab9 100644 --- a/test/models/maintenance_tasks/task_data_index_test.rb +++ b/test/models/maintenance_tasks/task_data_index_test.rb @@ -9,6 +9,7 @@ class TaskDataIndexTest < ActiveSupport::TestCase "Maintenance::BatchImportPostsTask", "Maintenance::CallbackTestTask", "Maintenance::CancelledEnqueueTask", + "Maintenance::CustomEnumeratingTask", "Maintenance::EnqueueErrorTask", "Maintenance::ErrorTask", "Maintenance::ImportPostsTask", diff --git a/test/models/maintenance_tasks/task_data_show_test.rb b/test/models/maintenance_tasks/task_data_show_test.rb index c181d4a22..6931e6d7d 100644 --- a/test/models/maintenance_tasks/task_data_show_test.rb +++ b/test/models/maintenance_tasks/task_data_show_test.rb @@ -87,6 +87,7 @@ class TaskDataShowTest < ActiveSupport::TestCase "date_attr", "time_attr", "boolean_attr", + "enum_attr", ], TaskDataShow.new("Maintenance::ParamsTask").parameter_names, ) diff --git a/test/models/maintenance_tasks/task_test.rb b/test/models/maintenance_tasks/task_test.rb index 62a9ca0c1..5911f2e2f 100644 --- a/test/models/maintenance_tasks/task_test.rb +++ b/test/models/maintenance_tasks/task_test.rb @@ -4,11 +4,12 @@ module MaintenanceTasks class TaskTest < ActiveSupport::TestCase - test ".available_tasks returns list of tasks that inherit from the Task superclass" do + test ".load_all returns list of tasks that inherit from the Task superclass" do expected = [ "Maintenance::BatchImportPostsTask", "Maintenance::CallbackTestTask", "Maintenance::CancelledEnqueueTask", + "Maintenance::CustomEnumeratingTask", "Maintenance::EnqueueErrorTask", "Maintenance::ErrorTask", "Maintenance::ImportPostsTask", @@ -23,7 +24,18 @@ class TaskTest < ActiveSupport::TestCase "Maintenance::UpdatePostsThrottledTask", ] assert_equal expected, - MaintenanceTasks::Task.available_tasks.map(&:name).sort + MaintenanceTasks::Task.load_all.map(&:name).sort + end + + test ".available_tasks raises a deprecation warning before calling .load_all" do + expected_warning = + "MaintenanceTasks::Task.available_tasks is deprecated and will be " \ + "removed from maintenance-tasks 3.0.0. Use .load_all instead.\n" + + Warning.expects(:warn).with(expected_warning, category: :deprecated) + Task.expects(:load_all) + + Task.available_tasks end test ".named returns the task based on its name" do diff --git a/test/system/maintenance_tasks/runs_test.rb b/test/system/maintenance_tasks/runs_test.rb index 7e316c949..7e370138b 100644 --- a/test/system/maintenance_tasks/runs_test.rb +++ b/test/system/maintenance_tasks/runs_test.rb @@ -7,12 +7,57 @@ class RunsTest < ApplicationSystemTestCase test "run a Task" do visit maintenance_tasks_path - click_on("Maintenance::UpdatePostsTask") - click_on "Run" + assert_difference("Run.count") do + click_on("Maintenance::UpdatePostsTask") + click_on "Run" - assert_title "Maintenance::UpdatePostsTask" - assert_text "Enqueued" - assert_text "Waiting to start." + assert_title "Maintenance::UpdatePostsTask" + assert_text "Enqueued" + assert_text "Waiting to start." + end + run = Run.last + assert_nil run.metadata + assert_equal "Maintenance::UpdatePostsTask", run.task_name + assert_equal "enqueued", run.status + end + + test "run a Task and log the provided metadata" do + MaintenanceTasks.metadata = -> { { user_email: "michael.elfassy@shopify.com" } } + visit(maintenance_tasks_path) + + assert_difference("Run.count") do + click_on("Maintenance::UpdatePostsTask") + click_on("Run") + + assert_title("Maintenance::UpdatePostsTask") + assert_text("Enqueued") + assert_text("Waiting to start.") + assert_text("Metadata") + assert_table do |table| + table.assert_text("user_email") + table.assert_text("michael.elfassy@shopify.com") + end + end + run = Run.last + assert_equal("michael.elfassy@shopify.com", run.metadata["user_email"]) + assert_equal("Maintenance::UpdatePostsTask", run.task_name) + assert_equal("enqueued", run.status) + ensure + MaintenanceTasks.metadata = nil + end + + test "metadata can be non-hash" do + MaintenanceTasks.metadata = -> { "hello metadata" } + visit(maintenance_tasks_path) + + assert_difference("Run.count") do + click_on("Maintenance::UpdatePostsTask") + click_on("Run") + + assert_text("hello metadata") + end + ensure + MaintenanceTasks.metadata = nil end test "run a CSV Task" do @@ -32,10 +77,10 @@ class RunsTest < ApplicationSystemTestCase click_on("Maintenance::ParamsTask") post_id = Post.first.id - fill_in("_task_arguments_post_ids", with: post_id.to_s) + fill_in("task[post_ids]", with: post_id.to_s) click_on "Run" - fill_in("_task_arguments_post_ids", with: "42") + fill_in("task[post_ids]", with: "42") perform_enqueued_jobs assert_title "Maintenance::ParamsTask" @@ -46,14 +91,14 @@ class RunsTest < ApplicationSystemTestCase table.assert_text("post_ids") table.assert_text(post_id.to_s) end - assert has_field?("_task_arguments_post_ids", with: "42") + assert has_field?("task[post_ids]", with: "42") end test "errors for Task with invalid arguments shown" do visit maintenance_tasks_path click_on("Maintenance::ParamsTask") - fill_in("_task_arguments_post_ids", with: "xyz") + fill_in("task[post_ids]", with: "xyz") click_on "Run" assert_text "Validation failed: Arguments are invalid: :post_ids is invalid" @@ -137,7 +182,7 @@ class RunsTest < ApplicationSystemTestCase assert_text "Cancelling…" refute_button "Cancel" - travel Run::STUCK_TASK_TIMEOUT + travel MaintenanceTasks.stuck_task_duration refresh click_on "Cancel" @@ -165,6 +210,11 @@ class RunsTest < ApplicationSystemTestCase assert_text "ArgumentError" assert_text "Something went wrong" assert_text "app/tasks/maintenance/error_task.rb:10:in `process'" + + click_on "Resume" + + assert_text "Enqueued" + assert_text "Waiting to start." end test "errors for double enqueue are shown" do diff --git a/test/system/maintenance_tasks/tasks_test.rb b/test/system/maintenance_tasks/tasks_test.rb index 7b7a93f9f..8f41db4c3 100644 --- a/test/system/maintenance_tasks/tasks_test.rb +++ b/test/system/maintenance_tasks/tasks_test.rb @@ -25,6 +25,7 @@ class TasksTest < ApplicationSystemTestCase "Maintenance::BatchImportPostsTask\nNew", "Maintenance::CallbackTestTask\nNew", "Maintenance::CancelledEnqueueTask\nNew", + "Maintenance::CustomEnumeratingTask\nNew", "Maintenance::EnqueueErrorTask\nNew", "Maintenance::ErrorTask\nNew", "Maintenance::Nested::NestedMore::NestedMoreTask\nNew", @@ -63,9 +64,8 @@ class TasksTest < ApplicationSystemTestCase assert_text "Paused" assert_equal ["Active Runs", "Previous Runs"], page.all("h4").map(&:text) - runs = page.all("h5").map(&:text) - assert_includes runs, "July 18, 2022 11:05\nPaused" - assert_includes runs, "January 01, 2020 01:00\nSucceeded" + assert_text(/July 18, 2022 11:05\nPaused\n#\d/) + assert_text(/January 01, 2020 01:00\nSucceeded\n#\d/) end test "task with attributes renders default values on the form" do @@ -73,9 +73,9 @@ class TasksTest < ApplicationSystemTestCase click_on("Maintenance::ParamsTask") - content_text = page.find_field("[task_arguments][content]").text + content_text = page.find_field("task[content]").text assert_equal("default content", content_text) - integer_attr_val = page.find_field("[task_arguments][integer_attr]").value + integer_attr_val = page.find_field("task[integer_attr]").value assert_equal("111222333", integer_attr_val) end @@ -84,36 +84,40 @@ class TasksTest < ApplicationSystemTestCase click_on("Maintenance::ParamsTask") - content_field = page.find_field("[task_arguments][content]") + content_field = page.find_field("task[content]") assert_equal("textarea", content_field.tag_name) - integer_field = page.find_field("[task_arguments][integer_attr]") + integer_field = page.find_field("task[integer_attr]") assert_equal("input", integer_field.tag_name) assert_equal("number", integer_field[:type]) assert_empty(integer_field[:step]) - big_integer_field = page.find_field("[task_arguments][big_integer_attr]") + big_integer_field = page.find_field("task[big_integer_attr]") assert_equal("input", big_integer_field.tag_name) assert_equal("number", big_integer_field[:type]) assert_empty(big_integer_field[:step]) - float_field = page.find_field("[task_arguments][float_attr]") + float_field = page.find_field("task[float_attr]") assert_equal("input", float_field.tag_name) assert_equal("number", float_field[:type]) assert_equal("any", float_field[:step]) - decimal_field = page.find_field("[task_arguments][decimal_attr]") + decimal_field = page.find_field("task[decimal_attr]") assert_equal("input", decimal_field.tag_name) assert_equal("number", decimal_field[:type]) assert_equal("any", decimal_field[:step]) - datetime_field = page.find_field("[task_arguments][datetime_attr]") + datetime_field = page.find_field("task[datetime_attr]") assert_equal("input", datetime_field.tag_name) assert_equal("datetime-local", datetime_field[:type]) - date_field = page.find_field("[task_arguments][date_attr]") + date_field = page.find_field("task[date_attr]") assert_equal("input", date_field.tag_name) assert_equal("date", date_field[:type]) - time_field = page.find_field("[task_arguments][time_attr]") + time_field = page.find_field("task[time_attr]") assert_equal("input", time_field.tag_name) assert_equal("time", time_field[:type]) - boolean_field = page.find_field("[task_arguments][boolean_attr]") + boolean_field = page.find_field("task[boolean_attr]") assert_equal("input", boolean_field.tag_name) assert_equal("checkbox", boolean_field[:type]) + enum_field = page.find_field("task[enum_attr]") + assert_equal("select", enum_field.tag_name) + enum_field_options = enum_field.find_all("option").map { |option| option[:value] } + assert_equal(["", "100", "200", "300"], enum_field_options) end test "view a Task with multiple pages of Runs" do diff --git a/test/test_helper.rb b/test/test_helper.rb index 898a8bae2..45b388b2d 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -3,6 +3,10 @@ # Configure Rails Environment ENV["RAILS_ENV"] = "test" +# Force Thor CLI to output on 200 columns, ignoring +# the size of the actual terminal running the tests. +ENV["THOR_COLUMNS"] = "200" + require_relative "../test/dummy/config/environment" ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)] @@ -43,6 +47,8 @@ module Warning class << self def warn(message) + return super if message.start_with?("Rack::Handler is deprecated") + raise message.to_s end end diff --git a/test/validators/maintenance_tasks/run_status_validator_test.rb b/test/validators/maintenance_tasks/run_status_validator_test.rb index ee617c7db..c584605cd 100644 --- a/test/validators/maintenance_tasks/run_status_validator_test.rb +++ b/test/validators/maintenance_tasks/run_status_validator_test.rb @@ -21,7 +21,7 @@ class RunStatusValidatorTest < ActiveSupport::TestCase assert_no_invalid_transitions([:enqueued, :interrupted], :running) end - test "run can go from paused to enqueued" do + test "run can go from paused or errored to enqueued" do paused_run = Run.create!( task_name: "Maintenance::UpdatePostsTask", status: :paused, @@ -30,7 +30,15 @@ class RunStatusValidatorTest < ActiveSupport::TestCase assert paused_run.valid? - assert_no_invalid_transitions([:paused], :enqueued) + errored_run = Run.create!( + task_name: "Maintenance::UpdatePostsTask", + status: :errored, + ) + errored_run.status = :enqueued + + assert errored_run.valid? + + assert_no_invalid_transitions([:paused, :errored], :enqueued) end test "run can go from running, cancelling or pausing to succeeded" do