Tests: Improve coverage

[mtancoigne]

🎩 Description

Improve tests to reach a global 94% code coverage.

Replaces #454 and #452 (issues with CI not triggering)

📌 Related Issues

No direct related issue

Testing

Run tests and check coverage :)

Tasks

Files under 94% coverage:

Checked	File	% covered	Lines	Relevant lines	Covered	Missed	Avg hit/line
ignored	lib/tasks/import.rake	3.77 %	226	106	4	102	0.04
deleted	app/jobs/decidim/machine_translation_resource_job.rb	18.52 %	120	54	10	44	0.19
split	lib/tasks/migrate.rake	25.66 %	234	113	29	84	0.26
-	app/services/decidim/s3_sync_service.rb	32.00 %	113	50	16	34	0.32
-	lib/extends/controllers/decidim/devise/account_controller_extends.rb	35.00 %	37	20	7	13	0.35
-	app/services/decidim/s3_retention_service.rb	38.24 %	79	34	13	21	0.38
100%	lib/tasks/repair_data.rake	58.54 %	73	41	24	17	0.83
100%	lib/tasks/db.rake	73.91 %	41	23	17	6	0.74
deleted	lib/active_storage/downloadable.rb	75.00 %	9	4	3	1	0.75
100%	app/helpers/decidim/backup_helper.rb	75.00 %	13	4	3	1	0.75
100%	app/services/decidim/action_log_service.rb	76.92 %	30	13	10	3	1.15
100%	lib/decidim/translator_configuration_helper.rb	77.78 %	19	9	7	2	0.89
100%	lib/decidim_app/sentry_setup.rb	80.00 %	55	30	24	6	5.83
-	app/services/dummy_authorization_handler.rb	84.62 %	107	26	22	4	2.50
100%	lib/decidim/rspec_runner.rb	87.50 %	63	32	28	4	2.31
100%	lib/decidim_app/decidim_initiatives.rb	93.18 %	81	44	41	3	1.32
100%	app/services/decidim/database_service.rb	93.33 %	55	30	28	2	2.50
100%	lib/tasks/decidim_app.rake	93.55 %	75	31	29	2	0.94

• lib/tasks/import.rake
  • Ignored for now : it looks like the task should not succeed in its current state
• lib/active_storage/downloadable.rb
  • Module is badly included in an attempt to override the default ActiveStorage::Blob's open method, in config/application.rb:

    ActiveStorage::Blob.instance_method(:open).source_location
    # => ["/<path>/activestorage-6.1.7.6/app/models/active_storage/blob.rb", 275]
    ActiveStorage::Blob.ancestors
    # => [ActiveStorage::Blob(Table doesn't exist), #<Module:0x000055619537a130>, ActiveStorage::Blob::Representable, ...]
    # Downloadable is not yet loaded
    
    ActiveStorage::Blob.include ActiveStorage::Downloadable
    ActiveStorage::Blob.instance_method(:open).source_location
    # => ["/<path>/activestorage-6.1.7.6/app/models/active_storage/blob.rb", 275]
    # It is still the original method.
    ActiveStorage::Blob.ancestors
    # => [ActiveStorage::Blob(Table doesn't exist), ActiveStorage::Downloadable, #<Module:0x000055619537a130>, ActiveStorage::Blob::Representable, ...]
    # "Downloadable" is _before_ the Blob class; "open()" is not overriden as declared _before_ the original one.
    
    # With "prepend" instead of "include"
    ActiveStorage::Blob.prepend ActiveStorage::Downloadable
    ActiveStorage::Blob.instance_method(:open).source_location
    # => ["/<path>/decidim-app/lib/active_storage/downloadable.rb", 5]
    ActiveStorage::Blob.ancestors
    # => [ActiveStorage::Downloadable, ActiveStorage::Blob, ...]

    That being said, using the right method leads to the test suite to fail. Both file and inclusion were removed from project.
• app/jobs/decidim/machine_translation_resource_job.rb
  • Duplicate file from decidim-core (decidim-core/app/jobs/decidim/machine_translation_resource_job.rb), tested in decidim-core/spec/jobs/decidim/machine_translation_resource_job_spec.rb. File was removed from the project.
• lib/tasks/migrate.rake
  • Classes extracted from task, then tested
  • There is still has a lot of procedural code which should be extracted to other files (RailsMigrations, maybe)
  • Task itself was not tested
• lib/extends/controllers/decidim/devise/account_controller_extends.rb
  • I don't figure how to get in the right context right now
• app/services/decidim/s3_sync_service.rb
  • #file_list output may be inconsistent: when a file list empty, the list is created from the backup directory and files are prefixed with the directory name. If the file list is given, no prefix is added.
    Tests were written to ensure this behavior, but still, it feels weird to me somehow.
• app/services/decidim/s3_retention_service.rb
  • #retention_dates Could be tested better with timecop gem to have reproducible results
  • Incomplete: #execute needs proper context and is not yet tested
  • Duplicated code with s3_sync_service.rb. I treated the files as if they were totally different ones (copy-pasted the common tests). A rework of the code of these two classes may be a good idea, with no duplicated tests too.
    I could have made some shared examples, but I prefer not to yet, as I don't know if the classes should be treated as totally independent.
• lib/tasks/repair_data.rake
• app/helpers/decidim/backup_helper.rb
  Question: What should be the behavior when outside a git repository ? Proposed test replacement:

  describe "#generate_subfolder_name" do
    context "without a Git repository" do
      it "raises an exception" do # instead of it "returns an incomplete string"
        FileUtils.cd File.dirname(temp_dir) do
          expect do
            generate_subfolder_name
          end.to raise_error
        end
      end
    end
  end

• app/services/dummy_authorization_handler.rb
  • This seems to be unused outside of tests. No additional test written.
• lib/tasks/decidim_app.rake
  • decidim_app:setup is explicitly ignored. Left as-is.
  • Tests checks that tasks don't fail; underlying code is already tested

EDIT

When comparing the number of files covered (52) vs the ruby files that should be covered (68), we end up with this list:

• app/channels/application_cable/channel.rb : Empty class; 0 direct usage/or reference in the code
• app/channels/application_cable/connection.rb : Empty class; 0 direct usage/or reference in the code
• app/jobs/backup_job.rb : referenced in sidekick.yml
• app/mailers/application_mailer.rb : 0 direct usage/or reference in the code
• app/models/application_record.rb : Only used in migrations (which are not tested)
• app/services/another_dummy_authorization_handler.rb : 0 direct usage/or reference in the code
• app/services/decidim/osp_authorization_handler.rb : String reference in config/initializers/decidim_verifications.rb. The tested code may never load the actual class.
• app/services/decidim/surveys_service.rb : Used in lib/tasks/db.rake (L. 32 et 37). My bet is those lines are not covered
• app/services/dummy_authorization_handler.rb : Used in spec/commands/decidim/verifications/authorize_user_spec.rb. Weird thing is: I opened that file at some point to decide not to do anything
• lib/extends/commands/decidim/admin/create_participatory_space_private_user_extends.rb : Used to monkey patch a Decidim class. May be never used in the tests
• lib/extends/commands/decidim/admin/impersonate_user_extends.rb : Used to monkey patch a Decidim class. May be never used in the tests
• lib/extends/queries/decidim/participatory_processes/group_participatory_processes_extends.rb : Used to monkey patch a Decidim class. May be never used in the tests

Simplecov, by default, ignores every files in which the specs don't enter, and there are two possibilities to have a worst coverage, but more accurate:

• Specify which files to include:

  # .simplecov
    SimpleCov.start do
    track_files '{app,lib}/**/*.rb'
    # ...
  end

• Load all the application by disabling eager loading:

  # config/environments/test.rb
  
  config.eager_load = false
  # or, as we only run Simplecov when the env var is set:
  config.eager_load = ENV['SIMPLECOV'] == '1'