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.

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

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 mayeut commented Feb 13, 2018

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

Copy link
Member

@latk latk left a comment

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 :)

@@ -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

@latk latk Feb 13, 2018

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

@mayeut mayeut Feb 13, 2018

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

@latk latk Feb 14, 2018

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.


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

@latk latk Feb 13, 2018

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

@latk latk Feb 13, 2018

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?

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

@latk latk Feb 13, 2018

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

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)! 👍

@@ -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

@latk latk Feb 13, 2018

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

@mayeut mayeut Feb 13, 2018

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

@mayeut mayeut Feb 13, 2018

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 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 mayeut commented Feb 13, 2018

I will reorganize as 3 distinct commits

mayeut added 3 commits Feb 13, 2018
Absolute filters are known to not work on Windows right now.
This prepares for future re-architecture of gcovr.
Use nosetests -v --with-coverage
@mayeut
Copy link
Contributor Author

@mayeut mayeut commented Feb 14, 2018

Fixes #213

latk
latk approved these changes Feb 14, 2018
@latk latk merged commit 5ac678b into gcovr:master Feb 14, 2018
2 checks passed
@latk latk removed the needs review label Feb 14, 2018
@mayeut mayeut deleted the module branch Feb 14, 2018
JamesReynolds pushed a commit to JamesReynolds/gcovr that referenced this issue Mar 8, 2018
Move script/gcovr to gcovr/__main__.py
latk pushed a commit to latk/gcovr that referenced this issue Mar 18, 2018
- 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
latk pushed a commit to latk/gcovr that referenced this issue Apr 6, 2018
- 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants