Skip to content

Conversation

ambrussimon
Copy link
Contributor

Closes #727 - declaring leftover coverage non-low-hanging
Closes #702 - perm handling got largely covered without explicit focus on the separate issue

  • Added a small number of safe no-cover pragmas
  • Added resolver, config, web.start and job text/html log coverage
  • Together w/ dataexplorer-tests (Add dataexplorer tests #847) coverage is at 91.26%

Leftovers

  • It's most efficient to target contiguous chunks
  • There are no obvious large chunks of missed statements left
  • Progress estimate from here on is 0.5% / day (around 30 lines)

Going forward - enforcement, CI integration

Ryan suggested adding no-cover to missed statements. My only addition is to add an extra tag to enable revisiting these in the future - eg. #pragma no-cover 100%
Pros:

  • having "100%" allows for very simple coverage enforcement going forward
  • works the same way locally and on CI

Con:

  • there's a chance that these "hidden" lines will never be revisited again

I've also considered the incremental approach (comparing to previous commit, or to master), but found way more cons:

  • harder (slower) to implement correctly
  • needs state (at least two runs)
  • local testing/CI differences more likely
  • doesn't get rid of the 'deleting covered code can decrease overall cov' problem

The current coverage-miss, sorted by missed-lines-per-file (skipping completely covered ones):

Module statements missing excluded coverage
api/dao/hierarchy.py 386 42 14 89%
api/dao/containerstorage.py 249 28 0 89%
api/handlers/listhandler.py 311 28 0 91%
api/handlers/containerhandler.py 375 23 0 94%
api/handlers/reporthandler.py 393 23 0 94%
api/jobs/gears.py 95 22 0 77%
api/auth/listauth.py 78 21 0 73%
api/handlers/collectionshandler.py 138 20 0 86%
api/jobs/queue.py 108 20 0 81%
api/placer.py 329 20 21 94%
api/dao/base.py 129 19 0 85%
api/dao/containerutil.py 118 19 0 84%
api/handlers/userhandler.py 141 19 0 87%
api/web/base.py 222 19 0 91%
api/jobs/rules.py 96 18 0 81%
api/auth/containerauth.py 97 15 0 85%
api/jobs/batch.py 124 15 0 88%
api/handlers/refererhandler.py 169 14 0 92%
api/tempdir.py 58 14 0 76%
api/upload.py 118 14 0 88%
api/handlers/dataexplorerhandler.py 196 11 0 94%
api/auth/groupauth.py 43 10 0 77%
api/config.py 142 10 0 93%
api/handlers/grouphandler.py 83 10 0 88%
api/auth/userauth.py 35 9 0 74%
api/dao/liststorage.py 137 9 0 93%
api/download.py 241 8 0 97%
api/web/start.py 52 8 12 85%
api/jobs/jobs.py 154 7 0 95%
api/validators.py 97 7 0 93%
api/dao/consistencychecker.py 35 5 0 86%
api/auth/authproviders.py 162 3 2 98%
api/dao/dbutil.py 15 3 0 80%
api/jobs/handlers.py 313 3 22 99%
api/util.py 117 3 5 97%
api/auth/__init__.py 48 2 0 96%
api/dao/__init__.py 16 2 0 88%
api/files.py 84 2 0 98%
api/handlers/devicehandler.py 49 1 2 98%
api/web/encoder.py 25 1 0 96%
Total 6030 527 80 91%

Review Checklist

  • Tests were added to cover all code changes
  • Documentation was added / updated
  • Code and tests follow standards in CONTRIBUTING.md

@ambrussimon ambrussimon self-assigned this Jul 18, 2017
c_metadata = self.metadata.get(self.container_type, {})
if self.context.get('job_id') and c_metadata and not c_metadata.get('files', []):
c_metadata = self.metadata.get(self.container_type, {}) # pragma: no cover
if self.context.get('job_id') and c_metadata and not c_metadata.get('files', []): # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we are still waiting on the classifier update I believe, as it was updated to use the classification tag.

# Start coverage before local module loading so their def and imports are counted
# http://coverage.readthedocs.io/en/coverage-4.2/faq.html
if os.environ.get("SCITRAN_RUNTIME_COVERAGE") == "true":
if os.environ.get("SCITRAN_RUNTIME_COVERAGE") == "true": # pragma: no cover - oh, the irony
Copy link
Contributor

Choose a reason for hiding this comment

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

😂😂😂😂

Copy link
Contributor

@nagem nagem left a comment

Choose a reason for hiding this comment

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

LGTM but @kofalt should look over resolver tests to make sure I didn't miss anything.

if x['node_type'] == "file" and x.get('name') == criterion:
return x, FileNode
raise Exception('No ' + criterion + ' acquisition or file found.')
raise Exception('No ' + criterion + ' file found.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find.

Copy link
Contributor

@kofalt kofalt left a comment

Choose a reason for hiding this comment

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

LGTM

@ambrussimon ambrussimon merged commit 21f58b6 into master Jul 25, 2017
@ambrussimon ambrussimon deleted the increase-coverage-low-hanging-fruit-7 branch July 25, 2017 10:20
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.

Increase coverage - low hanging fruit Add tests to cover permission handling

3 participants