Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move script/gcovr to gcovr/__main__.py #214

Merged
merged 3 commits into from
Feb 14, 2018
Merged

Move script/gcovr to gcovr/__main__.py #214

merged 3 commits into from
Feb 14, 2018

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Feb 13, 2018

This allows to run coverage on the module quite easily.
It will also prepare for future re-architecture of gcovr.

@mayeut
Copy link
Contributor Author

mayeut commented Feb 13, 2018

To run coverage test: nosetests -v --with-coverage

latk
latk reviewed Feb 13, 2018
View reviewed changes
Copy link
Member

@latk latk left a comment

Choose a reason for hiding this comment

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

Thank you, this is a very valuable change! However, it is actually three changes:

  • Moving the script to __main__.
  • Collecting test coverage data on gcovr.
  • Disabling the filter2 test on Windows.

Can you split moving gcovr vs. collecting coverage into two separate commits? Each of those is important enough to have a commit of its own. The current commit message doesn't mention coverage at all on the summary in the first line.

About disabling filter2, I'm not sure why that's necessary?

I've also added a few comments below. Even though I'd like to polish this PR a bit before merging, this is a really great solution, and moves the project along much faster than I would have expected :)

gcovr/__main__.py Outdated
@@ -2309,7 +2308,9 @@ def build_filter(regex):
return re.compile(os.path.realpath(regex))


def main(options, args):
def main():
global options, args
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason why options needs to be global? While that's technically a precise translation of the status quo, that isn't the semantics I intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are still some functions that are expecting those variables to be defined at the global scope. Without this, gcovr fails.

Copy link
Member

Choose a reason for hiding this comment

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

OK, we will fix that in a future commit when different modules are extracted.

gcovr/tests/test_gcovr.py Outdated

if 'coverage' in sys.modules:
run_coverage = True
env['GCOVR'] = python_interpreter + ' -m coverage run --branch --parallel-mode -m gcovr'
Copy link
Member

Choose a reason for hiding this comment

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

Using the GCOVR environment variable like this is clever. I like it!

setup.cfg Outdated

[coverage:run]
branch = True
parallel = True
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment that the same options need to be set when building the command in test_gcovr.py?

setup.py
scripts=scripts
entry_points={
'console_scripts': [
'gcovr=gcovr.__main__:main',
Copy link
Member

Choose a reason for hiding this comment

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

Finally 🎉 :) By moving the whole script to __main__, you've managed to introduce entry_points much earlier than I would have expected!

Copy link
Contributor

@goriy goriy Feb 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

pip install on Windows automatically creates .exe wrapper in Scripts directory due to this change! It's great! No more manual .bat wrappers necessary (for cross-platform gcovr calls)! 👍

gcovr/tests/filter-test2/Makefile Outdated
@@ -2,8 +2,7 @@ CFLAGS= -fprofile-arcs -ftest-coverage -fPIC

BASE_OS:=$(shell uname | cut -d'-' -f1)
ifeq ($(BASE_OS),MSYS_NT)
#GCOVR_TEST_OPTIONS := -f $(shell cygpath -w "$$PWD")\main.cpp
GCOVR_TEST_OPTIONS := -f main.cpp
GCOVR_TEST_OPTIONS := -f $(shell cygpath -w "$$PWD")\main.cpp
Copy link
Member

Choose a reason for hiding this comment

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

Absolute filters are known to not work on Windows right now. I would rather have this test run with a relative filter on Windows than skipping it entirely.

Can you explain how this change is related to collecting coverage or to extracting __main__? If there is a need for this commit, OK. I'd otherwise prefer to not merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rebase was a bit to extensive... I had no preference between running a relative filter or skipping the test. I will re-enable the test as proposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: the test was not passing the correct options anyway before those commits. It worked with python 2.7 but failed with python 3.6 now that tests are run with the correct interpreter on windows.

@mayeut
Copy link
Contributor Author

mayeut commented Feb 13, 2018

example of coverage ran through CI: https://codecov.io/gh/mayeut/gcovr/list/module-coverage/

@mayeut
Copy link
Contributor Author

mayeut commented Feb 13, 2018

I will reorganize as 3 distinct commits

@mayeut
Use a relative filter for filter-test2 on Windows
4895cb7 
Absolute filters are known to not work on Windows right now.
@mayeut
Move script/gcovr to gcovr/__main__.py
27756c8 
This prepares for future re-architecture of gcovr.
@mayeut
Add support for running nose with coverage
5468fc2 
Use nosetests -v --with-coverage
@mayeut
Copy link
Contributor Author

mayeut commented Feb 14, 2018

Fixes #213

@latk latk merged commit 5ac678b into gcovr:master Feb 14, 2018
@latk latk removed the needs review label Feb 14, 2018
@latk latk mentioned this pull request Feb 14, 2018
Closed
@mayeut mayeut deleted the module branch February 14, 2018 22:23
JamesReynolds pushed a commit to JamesReynolds/gcovr that referenced this pull request Mar 8, 2018
@latk
Merge pull request gcovr#214 from mayeut/module
5893c9d 
Move script/gcovr to gcovr/__main__.py
latk added a commit to latk/gcovr that referenced this pull request Mar 18, 2018
@latk
feature parity with dev branch
2d5fe1d 
- the driver/main was extracted in gcovr#214
- bugfixes have been superseded on master branch
- "make clean" is useful but not strictly necessary
- Travis config was committed independently in 0afac3a
@latk latk mentioned this pull request Mar 18, 2018
Merged
latk added a commit to latk/gcovr that referenced this pull request Apr 6, 2018
@latk
feature parity with dev branch
0bae381 
- the driver/main was extracted in gcovr#214
- Modularization was implemented in gcovr#225
- bugfixes have been superseded on master branch
- "make clean" is useful but not strictly necessary
- Travis config was committed independently in 0afac3a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goriy goriy goriy left review comments

@latk latk latk approved these changes

Assignees
No one assigned
Labels
Type: Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mayeut @latk @goriy