-
-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix cucumber coverage #898
base: main
Are you sure you want to change the base?
Conversation
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
@mvz is there anything else that you can think of that this work needs before it can be merged in? |
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.
@mvz Linter issues should be all fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mscottford thanks for looking into this 👍
Please have a look at my comments.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also worth noting, that simplecov
takes some time to start, so that could throw the timing off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm .. maybe aruba-test-cli
inherits RUBYOPT
and therefore loads simplecov_setup
? That shouldn't happen...
def format(result) | ||
Dir[File.join(File.dirname(__FILE__), "../public/*")].each do |path| | ||
FileUtils.cp_r(path, asset_output_path) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'll give the SimpleFormatter
approach a try.
desc "Run the whole test suite." | ||
task test: [:spec, :cucumber] | ||
task test: [:clobber, :spec, :cucumber] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't normally be needed. Can you remove this commit please?
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you remove the changes to the timeouts (see my comment above) please keep the changes in this file.
I'm fixing the unrelated RuboCop offense in a different pull request. |
Summary
The code coverage collection of
cucumber
features was broken. This pull request fixes it.Before applying this change, running
bundle exec cucumber
against thearuba
main
branch resulted in the following message being written to the console:After applying this change, running
bundle exec cucumber
results in the following message being written to./coverage/summary.txt
:Details
This was fixed by applying a few different changes:
RUBYOPT
environment variable was changed to useprepend_environment_variable
.Before
hook was replaced with anAround
hook that callswith_environment
to preserve environment variable changes.RUBYOPT
environment variable.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.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 thearuba
codebase appeared to support code coverage collection viasimplecov
, so I used thearuba
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 thearuba
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
Checklist:
I'm pretty sure that none of the above ☝️ apply to this change. Please let me know if I'm wrong.