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

Add --base-dir option #287

Merged
merged 8 commits into from May 24, 2021
Merged

Add --base-dir option #287

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5b1cd92
add --base-dir option (#1)
swryan Apr 3, 2021
f6993bc
Merge branch 'master' into master
swryan May 6, 2021
77b8c4d
tests
swryan May 6, 2021
a8a6b53
new file
swryan May 7, 2021
e0811ab
more tests for --base_dir
swryan May 7, 2021
4e65569
lint
swryan May 7, 2021
e71c7e4
change --base_dir to --basedir, default to ''
swryan May 18, 2021
d7a384b
Merge branch 'master' into master
swryan May 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion coveralls/api.py
View file
Open in desktop
Expand Up @@ -223,7 +223,9 @@ def wear(self, dry_run=False):
json_string = self.create_report()
if dry_run:
return {}
return self.submit_report(json_string)

def submit_report(self, json_string):
endpoint = '{}/api/v1/jobs'.format(self._coveralls_host.rstrip('/'))
verify = not bool(os.environ.get('COVERALLS_SKIP_SSL_VERIFY'))
response = requests.post(endpoint, files={'json_file': json_string},
Expand Down Expand Up @@ -368,7 +370,8 @@ def get_coverage(self):
workman.load()
workman.get_data()

return CoverallReporter(workman, workman.config).coverage
base_dir = self.config.get('base_dir', None)
Copy link
Owner

Choose a reason for hiding this comment

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

(see below)

Suggested change
base_dir = self.config.get('base_dir', None)
base_dir = self.config.get('base_dir') or ''

return CoverallReporter(workman, workman.config, base_dir).coverage

@staticmethod
def debug_bad_encoding(data):
Expand Down
15 changes: 12 additions & 3 deletions coveralls/cli.py
View file
Open in desktop
Expand Up @@ -19,7 +19,9 @@
Global options:
--service=<name> Provide an alternative service name to submit.
--rcfile=<file> Specify configuration file. [default: .coveragerc]
--base_dir=<dir> Base directory that is removed from reported paths.
Copy link
Owner

Choose a reason for hiding this comment

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

Please rename to:

Suggested change
--base_dir=<dir> Base directory that is removed from reported paths.
--basedir=<dir> Base directory that is removed from reported paths.

I was going to think through a convention for hyphens vs underscores for this, only to realize that was a silly level of overkill when we can just side-step any future consistency issues.

--output=<file> Write report to file. Doesn't send anything.
--submit=<file> Upload a previously generated file.
--merge=<file> Merge report from file when submitting.
--finish Finish parallel jobs.
-h --help Display this help.
Expand Down Expand Up @@ -60,7 +62,8 @@ def main(argv=None):
try:
coverallz = Coveralls(token_required,
config_file=options['--rcfile'],
service_name=options['--service'])
service_name=options['--service'],
base_dir=options.get('--base_dir', None))
Copy link
Owner

Choose a reason for hiding this comment

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

To keep up with the below null vs string comments:

Suggested change
base_dir=options.get('--base_dir', None))
base_dir=options.get('--base_dir') or '')


if options['--merge']:
coverallz.merge(options['--merge'])
Expand All @@ -75,6 +78,11 @@ def main(argv=None):
coverallz.save_report(options['--output'])
return

if options['--submit']:
with open(options['--submit']) as report_file:
coverallz.submit_report(report_file.read())
return

if options['--finish']:
log.info('Finishing parallel jobs...')
coverallz.parallel_finish()
Expand All @@ -86,8 +94,9 @@ def main(argv=None):

log.info('Coverage submitted!')
log.debug(result)
log.info(result['message'])
log.info(result['url'])
if result:
log.info(result.get('message', None))
log.info(result.get('url', None))
Copy link
Owner

Choose a reason for hiding this comment

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

nit: dict.get already returns None by default, no need to specify it.

Suggested change
log.info(result.get('message', None))
log.info(result.get('url', None))
log.info(result.get('message'))
log.info(result.get('url'))

except KeyboardInterrupt: # pragma: no cover
log.info('Aborted')
except CoverallsException as e:
Expand Down
12 changes: 11 additions & 1 deletion coveralls/reporter.py
View file
Open in desktop
Expand Up @@ -12,8 +12,14 @@
class CoverallReporter:
"""Custom coverage.py reporter for coveralls.io."""

def __init__(self, cov, conf):
def __init__(self, cov, conf, base_dir=None):
self.coverage = []
if base_dir:
self.base_dir = base_dir.replace(os.path.sep, '/')
if self.base_dir[-1] != '/':
self.base_dir += '/'
else:
self.base_dir = None
Copy link
Owner

Choose a reason for hiding this comment

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

nit:

Suggested change
def __init__(self, cov, conf, base_dir=None):
self.coverage = []
if base_dir:
self.base_dir = base_dir.replace(os.path.sep, '/')
if self.base_dir[-1] != '/':
self.base_dir += '/'
else:
self.base_dir = None
def __init__(self, cov, conf, base_dir=''):
self.coverage = []
self.base_dir = base_dir.replace(os.path.sep, '/')
if self.base_dir[-1] != '/':
self.base_dir += '/'

Keeping base_dir unconditionally as a string is a bit easier to reason about (fewer cases to enumerate) and will allow us to simplify the handling both here and in parse_file(), which no longer needs to have as many conditional checks (see comment below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need the if base_dir test regardless or the index operation will fail, so I thought it better to just do the minimal amount of work (the assignment) in the common case... no need to do the replace or the indexing operations 99% of the time

>>> base_dir = ''
>>> if base_dir[-1] != '/':
...     base_dir += '/'
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: string index out of range

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, good call, makes sense to leave as you have it (though the above example could be replaced with if not base_dir.endswith('/') to avoid that IndexError, FYI!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya that's a good point.. I guess I thought checking that single last char was more direct than using a string func, but I see the advantage

thanks for the thorough review

Copy link
Owner

Choose a reason for hiding this comment

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

FWIW you are correct and the direct indexing is faster:

~ » python3 -m timeit -s "x = 'asdf'" "x.endswith('f')"
2000000 loops, best of 5: 120 nsec per loop
~ » python3 -m timeit -s "x = 'asdf'" "x[-1] == 'f'"
5000000 loops, best of 5: 46.3 nsec per loop
~ » python3 -m timeit -s "x = 'asdf'" "x and x[-1] == 'f'"
5000000 loops, best of 5: 54.3 nsec per loop

~ » python3 -m timeit -s "x = ''" "x.endswith('f')"
2000000 loops, best of 5: 112 nsec per loop
~ » python3 -m timeit -s "x = ''" "x and x[-1] == 'f'"
20000000 loops, best of 5: 16.7 nsec per loop

...however at least personally I find that it's not always worth picking this most performant code for a block like this if another option would be more readable etc. Given this app tends to run once on CI with no real latency concerns, a couple hundred nanoseconds here and there is effectively ignorable :)

self.report(cov, conf)

def report5(self, cov):
Expand Down Expand Up @@ -183,9 +189,13 @@ def get_arcs(analysis):
def parse_file(self, cu, analysis):
"""Generate data for single file."""
filename = cu.relative_filename()

# ensure results are properly merged between platforms
posix_filename = filename.replace(os.path.sep, '/')

if self.base_dir and posix_filename.startswith(self.base_dir):
TheKevJames marked this conversation as resolved.
Show resolved Hide resolved
posix_filename = posix_filename[len(self.base_dir):]

source = analysis.file_reporter.source()

token_lines = analysis.file_reporter.source_token_lines()
Expand Down
1 change: 1 addition & 0 deletions example/example.json
View file
Open in desktop
@@ -0,0 +1 @@
{"source_files": [{"name": "coveralls-python/example/project.py", "source": "def hello():\n print('world')\n\n\nclass Foo:\n \"\"\" Bar \"\"\"\n\n\ndef baz():\n print('this is not tested')\n\ndef branch(cond1, cond2):\n if cond1:\n print('condition tested both ways')\n if cond2:\n print('condition not tested both ways')\n", "coverage": [1, 1, null, null, 1, null, null, null, 1, 0, null, 1, 1, 1, 1, 1]}, {"name": "coveralls-python/example/runtests.py", "source": "from project import branch\nfrom project import hello\n\nif __name__ == '__main__':\n hello()\n branch(False, True)\n branch(True, True)\n", "coverage": [1, 1, null, 1, 1, 1, 1]}], "service_name": "coveralls-python", "config_file": ".coveragerc", "base_dir": null}
107 changes: 107 additions & 0 deletions tests/api/reporter_test.py
View file
Open in desktop
Expand Up @@ -62,6 +62,113 @@ def test_reporter(self):
'name': 'runtests.py',
'coverage': [1, 1, None, 1, 1, 1, 1]})

def test_reporter_no_base_dir_arg(self):
subprocess.call(['coverage', 'run', '--omit=**/.tox/*',
'example/runtests.py'], cwd=BASE_DIR)

# without base_dir arg, file name is prefixed with 'example/'
os.chdir(BASE_DIR)
results = Coveralls(repo_token='xxx').get_coverage()
assert len(results) == 2

assert_coverage(results[0], {
'source': ('def hello():\n'
' print(\'world\')\n\n\n'
'class Foo:\n'
' """ Bar """\n\n\n'
'def baz():\n'
' print(\'this is not tested\')\n\n'
'def branch(cond1, cond2):\n'
' if cond1:\n'
' print(\'condition tested both ways\')\n'
' if cond2:\n'
' print(\'condition not tested both ways\')\n'),
'name': 'example/project.py',
'coverage': [1, 1, None, None, 1, None, None,
None, 1, 0, None, 1, 1, 1, 1, 1]})

assert_coverage(results[1], {
'source': ('from project import branch\n'
'from project import hello\n\n'
"if __name__ == '__main__':\n"
' hello()\n'
' branch(False, True)\n'
' branch(True, True)\n'),
'name': 'example/runtests.py',
'coverage': [1, 1, None, 1, 1, 1, 1]})

def test_reporter_with_base_dir_arg(self):
subprocess.call(['coverage', 'run', '--omit=**/.tox/*',
'example/runtests.py'], cwd=BASE_DIR)

# without base_dir arg, file name is prefixed with 'example/'
os.chdir(BASE_DIR)
results = Coveralls(repo_token='xxx',
base_dir='example').get_coverage()
assert len(results) == 2

assert_coverage(results[0], {
'source': ('def hello():\n'
' print(\'world\')\n\n\n'
'class Foo:\n'
' """ Bar """\n\n\n'
'def baz():\n'
' print(\'this is not tested\')\n\n'
'def branch(cond1, cond2):\n'
' if cond1:\n'
' print(\'condition tested both ways\')\n'
' if cond2:\n'
' print(\'condition not tested both ways\')\n'),
'name': 'project.py',
'coverage': [1, 1, None, None, 1, None, None,
None, 1, 0, None, 1, 1, 1, 1, 1]})

assert_coverage(results[1], {
'source': ('from project import branch\n'
'from project import hello\n\n'
"if __name__ == '__main__':\n"
' hello()\n'
' branch(False, True)\n'
' branch(True, True)\n'),
'name': 'runtests.py',
'coverage': [1, 1, None, 1, 1, 1, 1]})

def test_reporter_with_base_dir_trailing_sep(self):
subprocess.call(['coverage', 'run', '--omit=**/.tox/*',
'example/runtests.py'], cwd=BASE_DIR)

# without base_dir arg, file name is prefixed with 'example/'
os.chdir(BASE_DIR)
results = Coveralls(repo_token='xxx',
base_dir='example/').get_coverage()
assert len(results) == 2

assert_coverage(results[0], {
'source': ('def hello():\n'
' print(\'world\')\n\n\n'
'class Foo:\n'
' """ Bar """\n\n\n'
'def baz():\n'
' print(\'this is not tested\')\n\n'
'def branch(cond1, cond2):\n'
' if cond1:\n'
' print(\'condition tested both ways\')\n'
' if cond2:\n'
' print(\'condition not tested both ways\')\n'),
'name': 'project.py',
'coverage': [1, 1, None, None, 1, None, None,
None, 1, 0, None, 1, 1, 1, 1, 1]})

assert_coverage(results[1], {
'source': ('from project import branch\n'
'from project import hello\n\n'
"if __name__ == '__main__':\n"
' hello()\n'
' branch(False, True)\n'
' branch(True, True)\n'),
'name': 'runtests.py',
'coverage': [1, 1, None, 1, 1, 1, 1]})

def test_reporter_with_branches(self):
subprocess.call(['coverage', 'run', '--branch', '--omit=**/.tox/*',
'runtests.py'], cwd=EXAMPLE_DIR)
Expand Down
27 changes: 25 additions & 2 deletions tests/cli_test.py
View file
Open in desktop
Expand Up @@ -11,6 +11,10 @@
EXC = CoverallsException('bad stuff happened')


BASE_DIR = os.path.dirname(os.path.dirname(__file__))
EXAMPLE_DIR = os.path.join(BASE_DIR, 'example')


def req_json(request):
return json.loads(request.body.decode('utf-8'))

Expand Down Expand Up @@ -104,15 +108,17 @@ def test_real(mock_wear, mock_log):
def test_rcfile(mock_coveralls):
coveralls.cli.main(argv=['--rcfile=coveragerc'])
mock_coveralls.assert_called_with(True, config_file='coveragerc',
service_name=None)
service_name=None,
base_dir=None)


@mock.patch.dict(os.environ, {}, clear=True)
@mock.patch('coveralls.cli.Coveralls')
def test_service_name(mock_coveralls):
coveralls.cli.main(argv=['--service=travis-pro'])
mock_coveralls.assert_called_with(True, config_file='.coveragerc',
service_name='travis-pro')
service_name='travis-pro',
base_dir=None)


@mock.patch.object(coveralls.cli.log, 'exception')
Expand Down Expand Up @@ -142,3 +148,20 @@ def test_save_report_to_file_no_token(mock_coveralls):
"""Check save_report api usage when token is not set."""
coveralls.cli.main(argv=['--output=test.log'])
mock_coveralls.assert_called_with('test.log')


@mock.patch.object(coveralls.Coveralls, 'submit_report')
@mock.patch.dict(os.environ, {'TRAVIS': 'True'}, clear=True)
def test_submit(mock_submit):
json_file = os.path.join(EXAMPLE_DIR, 'example.json')
json_string = open(json_file).read()
coveralls.cli.main(argv=['--submit=' + json_file])
mock_submit.assert_called_with(json_string)


@mock.patch('coveralls.cli.Coveralls')
def test_base_dir_arg(mock_coveralls):
coveralls.cli.main(argv=['--base_dir=foo'])
mock_coveralls.assert_called_with(True, config_file='.coveragerc',
service_name=None,
base_dir='foo')