Fix cucumber coverage #898

mscottford · 2023-04-07T02:48:11Z

Summary

The code coverage collection of cucumber features was broken. This pull request fixes it.

Before applying this change, running bundle exec cucumber against the aruba main branch resulted in the following message being written to the console:

Coverage report generated for Cucumber Features to ./coverage. 0 / 0 LOC (100.0%) covered.

After applying this change, running bundle exec cucumber results in the following message being written to ./coverage/summary.txt:

Coverage report generated for Cucumber Features, [...list of features that were executed...] to ./coverage. 2375 / 2776 LOC (85.55%) covered.

Details

This was fixed by applying a few different changes:

The mechanism for modifying the RUBYOPT environment variable was changed to use prepend_environment_variable.
A Before hook was replaced with an Around hook that calls with_environment to preserve environment variable changes.
The mechanism for enabling coverage collection for sub-commands was changed slightly so that the file could be reliably required via the RUBYOPT environment variable.
The simplecov-html library was monkey-patched to work around its lack of support for suppressing the output that it generates. This was required to ensure that features pass when they are looking for the exact output of a command.
Timing values were changed in several features to account for the extra time required for Simplecov to do its work. The simplecov-html gem has an open issue that when fixed would likely permit the removal of this monkey patch. See Feature Request: Silence output_message simplecov-ruby/simplecov-html#116 for more information.

Motivation and Context

I was working on measuring code coverage for another project that I'm working on which uses aruba to test a Ruby-based CLI application. I noticed that the aruba codebase appeared to support code coverage collection via simplecov, so I used the aruba configuration as a starting point. When I discovered that the coverage collection was not working in that gem, and I later discovered that coverage collection was also failing the same way in the aruba repository, I dug in and figured out how to resolve the issue. That effort can be examined further by looking at CycloneDX/cyclonedx-ruby-gem#21.

How Has This Been Tested?

No changes were made to core aruba code, only the Simplecov integration and some timing values in features that rely on wait timeouts. All tests were passing locally (multiple times in a row) before opening the pull request. The situation may be different when things are run in a CI environment.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Internal change (refactoring, test improvements, developer experience or update of dependencies)

Checklist:

I've added tests for my code
My change requires a change to the documentation.
I have updated the documentation accordingly.

I'm pretty sure that none of the above ☝️ apply to this change. Please let me know if I'm wrong.

Note: `simplecov_setup` has been moved to it's own directory because of the way that it needs to be specified when setting the `RUBYOPTS` environment variable. Just specifying the path to the full file path via `-r` does not work. When doing that an error is generated that the file cannot be found. Instead, the directory containing the file is added to the load path via `-I`, and then the file is required with a simple `-rsimplecov_setup`. Because this modifies the load path, having a file named `aruba.rb` in the same directory as `simplecov_setup.rb` caused problems for files that attempted to `require 'aruba'`.

The Simplecov HTML formatter, which is the default, writes a message to the console after it has stored coverage information the `coverage` directory. There is no built in way to suppress this message. See simplecov-ruby/simplecov-html#116 and https://github.com/simplecov-ruby/simplecov-html/blob/v0.12.3/lib/simplecov-html.rb#L23..L32. The console output generated by this formatter causes failures for tests that are expecting exact command output. The approach employed here is to monkey patch the method so that it still generates the same message, but instead of writing to the console, it writes to a file in the `coverage` directory named `summary.txt`.

There are a few different reasons that we need to wait a little bit longer than we were doing so before. * On process start, there are more files that are being required, so we need to wait a little bit more. * During process execution, coverage data is being collected. * On process stop, there are files that are being written to the file system.

Coverage results get skewed when a `coverage` directory is already present

mscottford · 2023-07-07T13:32:58Z

I suspect that some of the changes in this pull request might benefit from the work that's being done in #894. Would it make any sense for me to lend a hand on #894 or is that a task that's best tackled by one person?

mscottford · 2023-07-07T13:35:06Z

@mvz is there anything else that you can think of that this work needs before it can be merged in?

mscottford · 2023-07-07T15:48:12Z

I'll fix the linter issues later today.

This addresses a linter error

This is based on a copy of the commit message from b9f98f8.

mscottford · 2023-07-07T17:04:02Z

@mvz Linter issues should be all fixed now.

mvz

Hi @mscottford thanks for looking into this 👍

Please have a look at my comments.

mvz · 2023-07-07T16:22:47Z

features/02_configure_aruba/basics.feature

@@ -55,7 +55,7 @@ Feature: Usage of configuration
    Given a file named "spec/support/aruba_config.rb" with:
    """ruby
    Aruba.configure do |config|
-      config.exit_timeout = 0.5
+      config.exit_timeout = 0.7


I'd rather avoid these changes to the timeouts. Aruba's tests suite takes a long time as it is. Also, I ran the full test suite without these timeout changes and there were no failures.

Are you able to run the test suite without failures from the main branch?

If I remember correctly, I increased the timeouts so that the tests would pass the test-windows (3.0) GitHub Action. The log from that run isn't showing up anymore when I take a look at it, so I'm not able to confirm that. I'll take out the changes and see what happens.

It's also worth noting, that simplecov takes some time to start, so that could throw the timing off.

Hm .. maybe aruba-test-cli inherits RUBYOPT and therefore loads simplecov_setup? That shouldn't happen...

mvz · 2023-07-07T16:30:20Z

features/support/simplecov/simplecov_setup.rb

+        def format(result)
+          Dir[File.join(File.dirname(__FILE__), "../public/*")].each do |path|
+            FileUtils.cp_r(path, asset_output_path)
+          end


This doesn't work because the original code refers to a public/ directory inside simplecov-html. So, if you start with no coverage/ directory and just run a cucumber scenario, there will be no styling in the HTML result.

One way to fix this would be to prepend a module instead that overrides format with something that temporarily redirects $stdout.

However another option would be to set SimpleCov.formatter = SimpleCov::Formatter::SimpleFormatter when ENV["SIMPLECOV_COMMAND_NAME"] is set. That should avoid the message and the needless copying of files. Also, it will make the SimpleCov message be printed at the very end of the Cucumber run.

Good catch! I'll give the SimpleFormatter approach a try.

mvz · 2023-07-07T17:09:08Z

Rakefile

 desc "Run the whole test suite."
-task test: [:spec, :cucumber]
+task test: [:clobber, :spec, :cucumber]


This shouldn't normally be needed. Can you remove this commit please?

mvz · 2023-07-07T17:23:13Z

features/04_aruba_api/filesystem/use_fixtures.feature

@@ -10,7 +10,7 @@ Feature: Use fixtures in your tests
    Given I use a fixture named "cli-app"

  Scenario: Use a fixture for your tests
-    Given a file named "spec/fixture_spec.rb" with:
+    Given a file named "spec/fixture_a_spec.rb" with:


When you remove the changes to the timeouts (see my comment above) please keep the changes in this file.

mvz · 2023-07-07T17:31:20Z

I'm fixing the unrelated RuboCop offense in a different pull request.

mscottford added 6 commits April 5, 2023 21:02

Adjusts timings to account for extra work being done by SImplecov

f8ed82e

Removes coverage directory before running full test suite

edd5992

Coverage results get skewed when a `coverage` directory is already present

Merge branch 'cucumber:main' into fix-cucumber-coverage

f196a14

mscottford added 2 commits July 7, 2023 12:54

Switches to double quotes

0cbb23b

This addresses a linter error

Adds class level documentation

fb37cfe

This is based on a copy of the commit message from b9f98f8.

mvz requested changes Jul 7, 2023

View reviewed changes

mvz added the status: needs rework label Apr 1, 2024

mvz mentioned this pull request Jun 2, 2024

Fix SimpleCov integration #933

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cucumber coverage #898

Fix cucumber coverage #898

mscottford commented Apr 7, 2023

mscottford commented Jul 7, 2023

mscottford commented Jul 7, 2023

mscottford commented Jul 7, 2023

mscottford commented Jul 7, 2023

mvz left a comment

mvz Jul 7, 2023

mscottford Jul 7, 2023

mscottford Jul 7, 2023

mvz Jul 7, 2023

mvz Jul 7, 2023

mscottford Jul 7, 2023

mvz Jul 7, 2023

mvz Jul 7, 2023

mvz commented Jul 7, 2023

Fix cucumber coverage #898

Are you sure you want to change the base?

Fix cucumber coverage #898

Conversation

mscottford commented Apr 7, 2023

Summary

Details

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mscottford commented Jul 7, 2023

mscottford commented Jul 7, 2023

mscottford commented Jul 7, 2023

mscottford commented Jul 7, 2023

mvz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvz commented Jul 7, 2023