Skip to content

Conversation

@evanweible-wf
Copy link
Contributor

Issue

#42 Generating an HTML report for coverage collection requires that "lcov" is installed. Currently, if that dependency is missing, the coverage task will fail with an non-helpful message when trying to generate the HTML report.

Changes

Source:

  • Introduce the concept of "platform utils"
    • Utility methods for interacting with the state of the current project or platform
    • Built to allow mocking
    • hasImmediateDependency, isExecutableInstalled
  • Use isExecutableInstalled util to determine if "lcov" is installed
    • if not, API throw a descriptive exception before even running the coverage task to save time
    • CLI updated to catch this exception and display the helpful message to stderr
  • Some general cleanup of the coverage task
    • Create descriptive exception classes
    • Consolidate the start/run logic

Tests:

  • Three tests added, but unfortunately they will require two changes to actually work:
    • programmatic execution of tests instead of being executed via processes (the mocking won't carry over to new processes)
    • need to mock the genhtml command
      • this needs to be able to give access to the stdout and stderr process streams, otherwise it could be a simple async method call

Areas of Regression

  • HTML report generation for coverage

Testing

  • Verify that the coverage task fails early and explains how to install lcov if lcov is missing
    • brew uninstall lcov
    • ddev coverage --html
    • error message should explain issue with steps to resolve
  • Verify that the coverage task does not fail if lcov is missing when HTML report is not requested
    • brew uninstall lcov
    • ddev coverage --no-html
    • coverage runs successfully, no html report geneated
  • Verify that the coverage task succeeds when lcov is installed
    • brew install lcov
    • ddev coverage --html
    • coverage runs successfully, html report generated

Code Review

@trentgrover-wf
@maxwellpeterson-wf
@dustinlessard-wf
@jayudey-wf

@evanweible-wf evanweible-wf force-pushed the genhtml branch 3 times, most recently from 81624ad to 921cdb6 Compare August 27, 2015 05:32
README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need code markup here:

brew update
brew install lcov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'lcov' -> executable?

@jayudey-wf jayudey-wf changed the title Coverage: check for lcov dependency, fail helpfully CP-907 Coverage: check for lcov dependency, fail helpfully Aug 27, 2015
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems misplaced

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was intentional. I want to eventually have the report generation go through here since it's interacting with an executable, and so that it can be mocked. But it will require more than a simple Future return type because I need to be able to listen to the stdout/stderr streams.

@codecov-io
Copy link

Current coverage is 48.05%

Merging #53 into master will decrease coverage by -4.06% as of 04c19e2

Powered by Codecov. Updated on successful CI builds.

@trentgrover-wf
Copy link
Contributor

+1, but looks like you need to change the min codecov coverage setting

@maxwellpeterson-wf
Copy link
Member

+1

@evanweible-wf
Copy link
Contributor Author

@trentgrover-wf @maxwellpeterson-wf @dustinlessard-wf coverage thresholds lowered, all checks passing now.

@maxwellpeterson-wf
Copy link
Member

+1

@dustinlessard-wf
Copy link

+1, neat!

@evanweible-wf
Copy link
Contributor Author

@trentgrover-wf

@trentgrover-wf
Copy link
Contributor

+1
@jayudey-wf ready for merge

@jayudey-wf
Copy link
Contributor

QA Resource Approval: +10

  • Testing instruction
  • Dev +1's
  • Dev/QA +10 with detail of what was tested
    • verified that if lcov was not installed that a warning was presented in the console if attempting to generate html, verified that if lcov was not generated and not attempting to generate the html page that coverage could still be run and then with lcov included that coverage was working as expected
  • Unit test created/updated
  • All unit tests pass

Merging into master.

jayudey-wf added a commit that referenced this pull request Aug 28, 2015
CP-907 Coverage: check for lcov dependency, fail helpfully
@jayudey-wf jayudey-wf merged commit 0864f64 into master Aug 28, 2015
@evanweible-wf evanweible-wf deleted the genhtml branch August 31, 2015 15:41
@jayudey-wf
Copy link
Contributor

@Rosie run_merge_script

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants