-
Notifications
You must be signed in to change notification settings - Fork 213
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
Separate test files from main module and add CI #150
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[run] | ||
omit = fhirclient/models/* |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
name: CI | ||
on: | ||
pull_request: | ||
push: | ||
branches: | ||
- main | ||
|
||
# The goal here is to cancel older workflows when a PR is updated (because it's pointless work) | ||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }} | ||
cancel-in-progress: true | ||
|
||
jobs: | ||
unittest: | ||
name: unit tests | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"] | ||
|
||
steps: | ||
- uses: actions/checkout@v4 | ||
|
||
- name: Set up Python ${{ matrix.python-version }} | ||
uses: actions/setup-python@v5 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
cache: pip | ||
|
||
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install -r requirements.txt | ||
pip install pytest pytest-cov | ||
|
||
- name: Test with pytest | ||
run: | | ||
python -m pytest --cov=fhirclient --cov-report=xml | ||
|
||
- name: Log missing coverage | ||
run: | | ||
coverage report -m --skip-covered | ||
|
||
- name: Check coverage report | ||
if: github.ref != 'refs/heads/main' | ||
uses: orgoro/[email protected] | ||
with: | ||
coverageFile: coverage.xml | ||
token: ${{ secrets.GITHUB_TOKEN }} | ||
thresholdAll: .35 | ||
thresholdNew: 1 | ||
thresholdModified: 0 | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
*.pyc | ||
__pycache__ | ||
session_data | ||
/build/ | ||
/dist/ | ||
/*.egg-info/ | ||
|
||
# docs | ||
/docs | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,16 @@ | ||
#!/usr/bin/env python | ||
# -*- coding: utf-8 -*- | ||
# | ||
# Generated from FHIR {{ info.version }} on {{ info.date }}. | ||
# {{ info.year }}, SMART Health IT. | ||
|
||
# Generated from FHIR {{ info.version }}, SMART Health IT. | ||
Comment on lines
-4
to
+1
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this part of the test template so that as we make small tweaks, we don't get a million comment changes as a result. I figured it was still reasonable to have it for the actual shipped models? But for the tests, meh. |
||
|
||
import os | ||
import io | ||
import unittest | ||
import json | ||
from . import {{ class.module }} | ||
from .fhirdate import FHIRDate | ||
from fhirclient.models import {{ class.module }} | ||
from fhirclient.models.fhirdate import FHIRDate | ||
|
||
|
||
class {{ class.name }}Tests(unittest.TestCase): | ||
def instantiate_from(self, filename): | ||
datadir = os.environ.get('FHIR_UNITTEST_DATADIR') or '' | ||
datadir = os.path.join(os.path.dirname(__file__), '..', 'data', 'examples') | ||
with io.open(os.path.join(datadir, filename), 'r', encoding='utf-8') as handle: | ||
js = json.load(handle) | ||
self.assertEqual("{{ class.name }}", js["resourceType"]) | ||
|
@@ -36,7 +31,7 @@ def test{{ class.name }}{{ loop.index }}(self): | |
def impl{{ class.name }}{{ loop.index }}(self, inst): | ||
{%- for onetest in tcase.tests %} | ||
{%- if "str" == onetest.klass.name %} | ||
self.assertEqual(inst.{{ onetest.path }}, "{{ onetest.value|replace('\\n', '\\\\n')|replace('"', '\\"') }}") | ||
self.assertEqual(inst.{{ onetest.path }}, "{{ onetest.value|replace('\\', '\\\\')|replace('"', '\\"') }}") | ||
{%- else %}{% if "int" == onetest.klass.name or "float" == onetest.klass.name or "NSDecimalNumber" == onetest.klass.name %} | ||
self.assertEqual(inst.{{ onetest.path }}, {{ onetest.value }}) | ||
{%- else %}{% if "bool" == onetest.klass.name %} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This got moved - just most lines changed a bit (indenting) that it got detected as a delete/add instead. I could go back and do the moves and edits as two separate commits to keep history, but I only noticed this after the fact and am being lazy - push back if you'd like to keep the history. |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ def find_version(*file_paths): | |
license="APACHE2", | ||
author="SMART Platforms Team", | ||
author_email='[email protected]', | ||
packages=find_packages(exclude=['test*', '*_tests.py']), | ||
packages=find_packages(exclude=['tests*']), | ||
install_requires=['requests', 'isodate'], | ||
classifiers=[ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the contents of this get updated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of the classifiers? Almost certainly, yes. But I'd like to postpone that for a general "move to pyproject.toml" PR |
||
'Development Status :: 4 - Beta', | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
This folder holds test code and data. | ||
|
||
To re-generate the test models, run `./generate_models.sh` from the project root. | ||
|
||
To re-generate the example test data, after re-generating the models, | ||
run: | ||
- `git rm tests/data/examples/*` | ||
- `cp fhir-parser/downloads/*example*.json tests/data/examples/` | ||
- `git add tests/data/examples/*` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
{ | ||
"resourceType": "Account", | ||
"id": "ewg", | ||
"text": { | ||
"status": "generated", | ||
"div": "\u003cdiv xmlns\u003d\"http://www.w3.org/1999/xhtml\"\u003eInpatient Admission for Peter James Chalmers Account\u003c/div\u003e" | ||
}, | ||
"identifier": [ | ||
{ | ||
"system": "urn:oid:0.1.2.3.4.5.6.7", | ||
"value": "654321" | ||
} | ||
], | ||
"status": "active", | ||
"type": { | ||
"coding": [ | ||
{ | ||
"system": "http://terminology.hl7.org/CodeSystem/v3-ActCode", | ||
"code": "PBILLACCT", | ||
"display": "patient billing account" | ||
} | ||
], | ||
"text": "patient" | ||
}, | ||
"name": "Inpatient: Peter James Chalmers", | ||
"subject": [ | ||
{ | ||
"reference": "Patient/example", | ||
"display": "Peter James Chalmers" | ||
} | ||
], | ||
"servicePeriod": { | ||
"start": "2016-01-01", | ||
"end": "2016-06-30" | ||
}, | ||
"coverage": [ | ||
{ | ||
"coverage": { | ||
"reference": "Coverage/9876B1" | ||
}, | ||
"priority": 1 | ||
}, | ||
{ | ||
"coverage": { | ||
"reference": "Coverage/7546D" | ||
}, | ||
"priority": 2 | ||
} | ||
], | ||
"owner": { | ||
"reference": "Organization/f001", | ||
"display": "Burgers University Medical Center" | ||
}, | ||
"description": "Hospital charges", | ||
"guarantor": [ | ||
{ | ||
"party": { | ||
"reference": "RelatedPerson/benedicte", | ||
"display": "Bénédicte du Marché" | ||
}, | ||
"onHold": false, | ||
"period": { | ||
"start": "2016-01-01" | ||
} | ||
} | ||
], | ||
"meta": { | ||
"tag": [ | ||
{ | ||
"system": "http://terminology.hl7.org/CodeSystem/v3-ActReason", | ||
"code": "HTEST", | ||
"display": "test health data" | ||
} | ||
] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
{ | ||
"resourceType": "Account", | ||
"id": "example", | ||
"text": { | ||
"status": "generated", | ||
"div": "\u003cdiv xmlns\u003d\"http://www.w3.org/1999/xhtml\"\u003eHACC Funded Billing for Peter James Chalmers\u003c/div\u003e" | ||
}, | ||
"identifier": [ | ||
{ | ||
"system": "urn:oid:0.1.2.3.4.5.6.7", | ||
"value": "654321" | ||
} | ||
], | ||
"status": "active", | ||
"type": { | ||
"coding": [ | ||
{ | ||
"system": "http://terminology.hl7.org/CodeSystem/v3-ActCode", | ||
"code": "PBILLACCT", | ||
"display": "patient billing account" | ||
} | ||
], | ||
"text": "patient" | ||
}, | ||
"name": "HACC Funded Billing for Peter James Chalmers", | ||
"subject": [ | ||
{ | ||
"reference": "Patient/example", | ||
"display": "Peter James Chalmers" | ||
} | ||
], | ||
"servicePeriod": { | ||
"start": "2016-01-01", | ||
"end": "2016-06-30" | ||
}, | ||
"coverage": [ | ||
{ | ||
"coverage": { | ||
"reference": "Coverage/7546D" | ||
}, | ||
"priority": 1 | ||
} | ||
], | ||
"owner": { | ||
"reference": "Organization/hl7" | ||
}, | ||
"description": "Hospital charges", | ||
"meta": { | ||
"tag": [ | ||
{ | ||
"system": "http://terminology.hl7.org/CodeSystem/v3-ActReason", | ||
"code": "HTEST", | ||
"display": "test health data" | ||
} | ||
] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So hear me out. I'm excluding the generated models/* folder - which... I could believe I shouldn't. But I'm taking baby steps. The generated code does miss some stuff - import error handling and some small enum-style classes that don't get instantiated during the tests.
So I'm not super stressed about that coverage gap, and I'd rather come back to that later and focus on the coverage of the manual code we wrote for this repo. Which... isn't great:
So with this coverage config, I'm just surfacing the problem, and making sure it doesn't get much worse - but not really enforcing a ratchet upwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, i'm totally here for 'start with a bar you can easily clear and work back from there'