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

Separate test files from main module and add CI #150

Merged
merged 1 commit into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
2 changes: 2 additions & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[run]
omit = fhirclient/models/*
52 changes: 52 additions & 0 deletions .github/workflows/ci.yaml
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
Comment on lines +50 to +52
Copy link
Contributor Author

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:

File 	statements 	missing 	excluded 	coverage
fhirclient/__init__.py 	5 	0 	0 	100%
fhirclient/auth.py 	249 	138 	0 	45%
fhirclient/client.py 	138 	138 	0 	0%
fhirclient/server.py 	150 	74 	0 	51%
Total                  	542 	350 	0 	35%

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.

Copy link
Contributor

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'

3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
*.pyc
__pycache__
session_data
/build/
/dist/
/*.egg-info/

# docs
/docs
Expand Down
3 changes: 1 addition & 2 deletions fhir-parser-resources/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@

# unit tests
write_unittests = True
tpl_unittest_target = '../fhirclient/models' # target directory to write the generated unit test files to
tpl_unittest_target_ptrn = '{}_tests.py' # TODO: remove in future when we re-organize tests
tpl_unittest_target = '../tests/models' # target directory to write the generated unit test files to


# all these files should be copied to dirname(`tpl_resource_target_ptrn`): tuples of (path/to/file, module, array-of-class-names)
Expand Down
15 changes: 5 additions & 10 deletions fhir-parser-resources/template-unittest.py
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"])
Expand All @@ -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 %}
Expand Down
72 changes: 0 additions & 72 deletions fhirclient/server_tests.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1 change: 0 additions & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
[tool:pytest]
minversion = 2.5
python_files = *_tests.py

[wheel]
# Since we're a pure Python package, we can mark our wheels as universal.
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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=[
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the contents of this get updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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',
Expand Down
30 changes: 0 additions & 30 deletions test_models.sh

This file was deleted.

9 changes: 9 additions & 0 deletions tests/README.md
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/*`
Empty file added tests/__init__.py
Empty file.
File renamed without changes.
76 changes: 76 additions & 0 deletions tests/data/examples/account-example-with-guarantor.json
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"
}
]
}
}
57 changes: 57 additions & 0 deletions tests/data/examples/account-example.json
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"
}
]
}
}
Loading