Summary formatter should print more error information

[deivid-rodriguez]

Describe the bug

Hi! Not sure if this is intentional but I was expecting the summary formatter to format "success" the way it does, but in the case of errors, I was expecting to still give me useful information about the errors.

To Reproduce

• Clone https://github.com/activeadmin/activeadmin.

• Run bundle install.

• Introduce a typo bug like

  diff --git a/spec/support/templates/policies/post_policy.rb b/spec/support/templates/policies/post_policy.rb
  index 8d39cac13..8480071cc 100644
  --- a/spec/support/templates/policies/post_policy.rb
  +++ b/spec/support/templates/policies/post_policy.rb
  @@ -1,5 +1,5 @@
   class PostPolicy < ApplicationPolicy
     def update?
  -    record.author == user
  +    recorda.author == user
     end
   end

• Setup test environment with bin/rake setup.

• Run cucumber feature with bin/cucumber --format summary features/authorization_pundit.feature:21.

Expected behavior

I was expecting a more helpful error like the one you get with --format pretty:

$ bin/cucumber --format pretty features/authorization_pundit.feature:21 
Using the default profile...
@authorization
Feature: Authorizing Access using Pundit

  Background:                            # features/authorization_pundit.feature:4
    Given I am logged in                 # features/step_definitions/user_steps.rb:9
    And 1 post exists                    # features/step_definitions/factory_steps.rb:6
    And a configuration of:              # features/step_definitions/configuration_steps.rb:13
      """
      require 'pundit'

      ActiveAdmin.application.namespace(:admin).authorization_adapter = ActiveAdmin::PunditAdapter

      ActiveAdmin.register Post do
      end

      ActiveAdmin.register_page "No Access" do
      end
      """
    And I am on the index page for posts # features/step_definitions/web_steps.rb:52
      undefined local variable or method `recorda' for #<PostPolicy:0x0000558c61339148 @user=#<AdminUser id: 1, email: "[email protected]", created_at: "2021-03-25 17:23:41.114977000 +0000", updated_at: "2021-03-25 17:23:41.114977000 +0000">, @record=#<Post id: 1, title: "Hello World 0", body: nil, published_date: nil, author_id: nil, position: nil, custom_category_id: nil, starred: nil, foo_id: nil, created_at: "2021-03-25 17:23:41.141817000 +0000", updated_at: "2021-03-25 17:23:41.141817000 +0000">>
      Did you mean?  record
                     @record (ActionView::Template::Error)
      ./tmp/test_apps/rails_61/app/policies/post_policy.rb:3:in `update?'
      ./lib/active_admin/pundit_adapter.rb:17:in `public_send'
      ./lib/active_admin/pundit_adapter.rb:17:in `authorized?'
      ./lib/active_admin/base_controller/authorization.rb:38:in `authorized?'
      ./lib/active_admin/base_controller/authorization.rb:20:in `authorized?'
      ./lib/active_admin/views/index_as_table.rb:379:in `defaults'
      ./lib/active_admin/views/index_as_table.rb:362:in `block (2 levels) in actions'
      ./lib/active_admin/views/index_as_table.rb:361:in `block in actions'
      ./lib/active_admin/view_helpers/display_helper.rb:58:in `find_value'
      ./lib/active_admin/view_helpers/display_helper.rb:45:in `format_attribute'
      ./lib/active_admin/views/components/table_for.rb:102:in `block in build_table_cell'
      ./lib/active_admin/views/components/table_for.rb:101:in `build_table_cell'
      ./lib/active_admin/views/components/table_for.rb:45:in `block (2 levels) in column'
      ./lib/active_admin/views/components/table_for.rb:44:in `block in column'
      ./lib/active_admin/views/components/table_for.rb:43:in `each_with_index'
      ./lib/active_admin/views/components/table_for.rb:43:in `column'
      ./lib/active_admin/views/index_as_table.rb:354:in `actions'
      ./lib/active_admin/views/index_as_table.rb:264:in `block in default_table'
      ./lib/active_admin/views/index_as_table.rb:249:in `instance_exec'
      ./lib/active_admin/views/index_as_table.rb:249:in `block in build'
      ./lib/active_admin/views/components/table_for.rb:21:in `build'
      ./lib/active_admin/views/index_as_table.rb:254:in `table_for'
      ./lib/active_admin/views/index_as_table.rb:247:in `build'
      ./lib/active_admin/views/pages/index.rb:141:in `block (2 levels) in render_index'
      ./lib/active_admin/views/pages/index.rb:140:in `block in render_index'
      ./lib/active_admin/views/pages/index.rb:133:in `render_index'
      ./lib/active_admin/views/pages/index.rb:50:in `build_collection'
      ./lib/active_admin/views/pages/index.rb:28:in `block in main_content'
      ./lib/active_admin/views/pages/index.rb:36:in `wrap_with_batch_action_form'
      ./lib/active_admin/views/pages/index.rb:26:in `main_content'
      ./lib/active_admin/views/pages/base.rb:101:in `block (2 levels) in build_main_content_wrapper'
      ./lib/active_admin/views/pages/base.rb:100:in `block in build_main_content_wrapper'
      ./lib/active_admin/views/pages/base.rb:99:in `build_main_content_wrapper'
      ./lib/active_admin/views/pages/base.rb:83:in `block in build_page_content'
      ./lib/active_admin/views/pages/base.rb:82:in `build_page_content'
      ./lib/active_admin/views/pages/base.rb:59:in `block (2 levels) in build_page'
      ./lib/active_admin/views/pages/base.rb:55:in `block in build_page'
      ./lib/active_admin/views/pages/base.rb:54:in `build_page'
      ./lib/active_admin/views/pages/base.rb:9:in `build'
      ./app/views/active_admin/resource/index.html.arb:2:in `block in __home_deivid__ode_activeadmin_app_views_active_admin_resource_index_html_arb___1907912821212394310_26820'
      ./app/views/active_admin/resource/index.html.arb:1:in `new'
      ./app/views/active_admin/resource/index.html.arb:1:in `__home_deivid__ode_activeadmin_app_views_active_admin_resource_index_html_arb___1907912821212394310_26820'
      ./lib/active_admin/resource_controller/streaming.rb:12:in `index'
      ./features/step_definitions/web_steps.rb:53:in `/^I am on (.+)$/'
      features/authorization_pundit.feature:19:in `I am on the index page for posts'

  Scenario: Attempt to access a resource I am not authorized to see   # features/authorization_pundit.feature:21
    When I go to the last post's edit page                            # features/step_definitions/web_steps.rb:56
    Then I should see "You are not authorized to perform this action" # features/step_definitions/web_steps.rb:80

Failing Scenarios:
cucumber features/authorization_pundit.feature:21 # Scenario: Attempt to access a resource I am not authorized to see

1 scenario (1 failed)
6 steps (1 failed, 2 skipped, 3 passed)
0m0.607s

Randomized with seed 9856

However, with --format summary, all the error information is "swallowed", so issues are hard to debug:

$ bin/cucumber --format summary features/authorization_pundit.feature:21 
Authorizing Access using Pundit
  Attempt to access a resource I am not authorized to see ✗

Failing Scenarios:
cucumber features/authorization_pundit.feature:21 # Scenario: Attempt to access a resource I am not authorized to see

1 scenario (1 failed)
6 steps (1 failed, 2 skipped, 3 passed)
0m0.726s

Randomized with seed 18864

Context & Motivation

I introduced some bugs in my code that made cucumber scenarios failed, but I had no clue on how to go about investigating them until I realized changing the formatter would give me more useful information for debugging.

Your Environment

• Versions used [5.3.0]
• Operating System and version [Linux]

[luke-hill]

In short. Yes and No.

Yes it does print all you've mentioned, yes that's deliberate (For now), and yes it's slightly buggy (It doesn't report re-runs e.t.c. properly).

No it "shouldn't" do these things.

Now I'm not 100% sure, as I've been out of the loop RE formatters for a while. But I believe that our html / progress formatters use messages, and the rest I'm not sure on. So it "could" be the reason this is not as fully formed is due to it not using our protobuf.

I'll delegate to @aurelien-reeves / @vincent-psarga on this as they did a bunch of the work in this area I believe.

[deivid-rodriguez]

Implementation wise, from looking at the source, the impression I got is that it was just missing some logic and that it shouldn't be hard to implement.

But I wanted to ask first whether the change would be desired. The way I see it, it's hardly ever useful to swallow errors in this way.

[aurelien-reeves]

I have to check with other implem of cucumber and other tools
But my first guess would be that the summary is just that: a summary. When you have an error reported by a summary, it should not report more that it does.

It is actually explicitly documented that way: https://github.com/cucumber/cucumber-ruby/blob/master/features/docs/formatters/summary_formatter.feature

[luke-hill]

So for things like formatters, at the moment it's opportunity / loss.

We have 2 big ish things to consider.

1. If it's legacy, it would need rewriting into protobuf (For now, more on point 2).
2. We are currently speaking about migrating from protobuf to a JSON schema, which might make any large scale refactor work redundant. See here: Remove runtime dependency on Protobuf common#1386 for more details.

[deivid-rodriguez]

It is actually explicitly documented that way: https://github.com/cucumber/cucumber-ruby/blob/master/features/docs/formatters/summary_formatter.feature

Well, if by documentation you mean

This formatter mimics the output from tools like RSpec or Mocha, giving an overview of each feature and scenario but omitting the steps.

I actually interpret that as a sign that this should be changed because neither RSpec nor Mocha have this behaviour on failures, and it doesn't say anything about errors, only about omitting the steps.

If by documentation you mean that the scenario expectations would need to be changed because they currently explicitly expect that errors are silent, then, yeah, sure.

[deivid-rodriguez]

Also note that the progress formatter, which is even more concise and the natural port of the default RSpec formatter, prints errors as well.

[aurelien-reeves]

If by documentation you mean that the scenario expectations would need to be changed because they currently explicitly expect that errors are silent, then, yeah, sure.

Yes I meant that :)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two months if no further activity occurs.