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

Add --base-dir option #287

Merged
merged 8 commits into from May 24, 2021
Merged

Add --base-dir option #287

merged 8 commits into from May 24, 2021

Conversation

@swryan
Copy link
Contributor

@swryan swryan commented May 6, 2021

  • add --base-dir option
  • add --submit option

When testing an installed python package, the path to source files appears as the full site-packages path.

We originally addressed this using the --output option to capture and post-process the data, which then we could submit with the new --submit option.

A more direct solution was to add a --base_dir argument and do the path replacement within coveralls-python.

Both of these new options are included here for your consideration.

* add option to upload previously generated file

* cleanup

* fix

* add --base_dir arg

* result=None?

* fix

* win fix

* posix base_dir

* big

* fix tests
@swryan swryan requested a review from TheKevJames as a code owner May 6, 2021
@TheKevJames
Copy link
Owner

@TheKevJames TheKevJames commented May 17, 2021

Hi @swryan, thanks for the contribution! Could you expand a bit upon why a user might make use of this feature? I'm having a bit of trouble understanding what this flag would be used for. A quick example would be absolutely wonderful!

Loading

@swryan
Copy link
Contributor Author

@swryan swryan commented May 18, 2021

Sure... we use it in our workflow here:
https://github.com/OpenMDAO/OpenMDAO/blob/master/.github/workflows/openmdao_test_workflow.yml#L191

We like to run our CI tests with the installed package and in a directory outside the repo to catch any packaging errors (e.g. missing files), so coveralls.io would include the full installed path including site-packages, etc. The installed path of course varies across platforms, which also messes up the merged results.

So we generate the coverage and use the --base-dir arg to strip the site-packages part of the path:

          pip install git+https://github.com/swryan/coveralls-python
          SITE_DIR=`python -c 'import site; print(site.getsitepackages()[-1])'`
          coveralls --base_dir $SITE_DIR

As I mentioned before.. our first approach to this problem was to --output and post-process the data and then --submit it (we had originally named the option --upload):
https://github.com/OpenMDAO/OpenMDAO/blob/197e2810c30895719c5cdb51988b67308c63e715/.travis.yml#L175

    coveralls --rcfile=../../.coveragerc --output=coveralls.json;
    SITE=`python -c 'import site; print(site.getsitepackages()[0])'`;
    sed "s/${SITE//\//\\/}\///g" < coveralls.json > coveralls-upd.json;
    coveralls --upload=coveralls-upd.json;

These are just the solutions we came up with. If there's a better way, I'm all ears.

Loading

Copy link
Owner

@TheKevJames TheKevJames left a comment

Thanks for the example, that was very clear. I can't think of any other decent way to support this with existing options, so I'm definitely in favour of introducing a new approach.

I actually like both of these (--basedir and --upload) -- I can't think of a good reason not to introduce both. Added comments, mostly on the stylistic side of things, but overall the implementation looks great.

Loading

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

@TheKevJames TheKevJames May 18, 2021

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

Loading

Copy link
Contributor Author

@swryan swryan May 18, 2021

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

Loading

Copy link
Owner

@TheKevJames TheKevJames May 24, 2021

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

Loading

Copy link
Contributor Author

@swryan swryan May 25, 2021

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

Loading

Copy link
Owner

@TheKevJames TheKevJames May 25, 2021

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

Loading

coveralls/reporter.py Show resolved Hide resolved
Loading
coveralls/cli.py Outdated
@@ -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

@TheKevJames TheKevJames May 18, 2021

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

Loading

coveralls/cli.py Outdated
log.info(result.get('message', None))
log.info(result.get('url', None))
Copy link
Owner

@TheKevJames TheKevJames May 18, 2021

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

Loading

coveralls/api.py Outdated
@@ -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

@TheKevJames TheKevJames May 18, 2021

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

Loading

coveralls/cli.py Outdated
@@ -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

@TheKevJames TheKevJames May 18, 2021

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.

Loading

@TheKevJames TheKevJames merged commit 165a5cd into TheKevJames:master May 24, 2021
18 of 19 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants