Skip to content

Commit f49d0ee

Browse files
jr-rkclaude
andcommitted
test: add CLARIN consumer-contract coverage + branch-differential CI
The CLARIN/UFAL surface was merged onto dtq (main -> dtq, 2026-08-17) with zero tests, while three main-lineage consumers depend on it. This lands the counterpart to test_repo_usage_contract.py for that surface and makes the main+dtq merge validatable instead of assumed. Defects fixed (characterization-tested; each crashed or mis-answered a real consumer call before this change): - get_bundle_by_name / get_resource_policy / get_items_from_collection: guard the None from a failed fetch instead of a NoneType subscript (D1/D3/D6). - get_resource_policy: empty policy list -> None, not IndexError (D2). - get_user_by_email: a 404 miss -> None, not a truthy uuid-less User that slips past `if user:` (D4). - create_submit_group: a 201 with no body -> None, not Group(None) (D5). - Group(None) / User(None): construct an empty object, not TypeError (D7). - get_owningCollection: on non-200 return None (not an empty truthy Collection) so src/repo/_audit.py's `owning_col is None and last_err.status_code == 401` reauth retry actually fires (D8, found via the consumer contract). Tests: +69 (68 -> 137). New suites test_clarin_read/write, test_models_clarin, test_clarin_usage_contract, test_transport_hardening; _helpers gains CLARIN builders; TestGetItems gains the no-arg form; test_repo_usage_contract gains the owningCollection reauth contract. CI: tests.yml now builds on dtq/main/feat/**/fix/**; a differential-contract job runs the CLARIN suite against BOTH the dtq and main implementations (@pytest.mark.dtq_only marks the fixes/deltas skipped on the main leg) as the merge gate; a coverage floor (--cov-fail-under=70) and secret-gated consumer-smoke jobs are added. pytest.ini registers the markers. Verified locally: leg A (dtq) 137 pass; leg B (main impl) 45 shared contracts pass, 14 dtq_only deselected; the 14 dtq_only CLARIN tests fail on the main impl, confirming the deltas are real. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 68c4c42 commit f49d0ee

14 files changed

Lines changed: 1220 additions & 9 deletions

.github/workflows/tests.yml

Lines changed: 126 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@ name: Tests
22

33
on:
44
push:
5-
branches: [ dtq ]
5+
# dtq is the mainline; main is the CLARIN branch we are validating a merge
6+
# into. feat/** and fix/** get CI before they open a PR.
7+
branches: [ dtq, main, 'feat/**', 'fix/**' ]
68
pull_request:
9+
workflow_dispatch: # manual validation of a merge candidate
710

811
permissions:
912
contents: read
@@ -37,4 +40,125 @@ jobs:
3740
pip install -r requirements-test.txt
3841
3942
- name: Run tests
40-
run: python -m pytest tests/ -v
43+
# Coverage floor guards against the CLARIN surface silently sliding back
44+
# toward the zero it had before test/clarin-usage-coverage landed.
45+
run: >
46+
python -m pytest tests/ -v
47+
--cov=dspace_rest_client --cov-report=term-missing
48+
--cov-fail-under=70
49+
50+
differential-contract:
51+
# THE MERGE GATE. Run the CLARIN consumer-contract suite against BOTH the
52+
# dtq implementation (this checkout) and the main implementation (swapped in
53+
# from origin/main). A test green on both proves the merge preserves that
54+
# behaviour. Tests that deliberately encode a dtq fix or behaviour change
55+
# are marked @pytest.mark.dtq_only and are skipped on the main leg.
56+
#
57+
# Leg selection targets the four CLARIN test files explicitly: the DQ test
58+
# modules import dtq-only symbols (ResourcePolicy, ...) at module scope, so
59+
# collecting them against the main implementation would be an import error.
60+
name: CLARIN contract vs ${{ matrix.impl }} impl
61+
runs-on: ubuntu-latest
62+
strategy:
63+
fail-fast: false
64+
matrix:
65+
impl: [dtq, main]
66+
67+
steps:
68+
- uses: actions/checkout@v6
69+
with:
70+
fetch-depth: 0 # need origin/main to swap the implementation in
71+
72+
- name: Set up Python
73+
uses: actions/setup-python@v6
74+
with:
75+
python-version: "3.10"
76+
cache: pip
77+
cache-dependency-path: requirements-test.txt
78+
79+
- name: Install test deps
80+
run: |
81+
python -m pip install --upgrade pip
82+
pip install -r requirements-test.txt
83+
84+
- name: Swap in the ${{ matrix.impl }} implementation
85+
if: matrix.impl != 'dtq'
86+
run: git checkout "origin/${{ matrix.impl }}" -- dspace_rest_client/
87+
88+
- name: Run CLARIN contract suite
89+
run: |
90+
FILES="tests/test_clarin_read.py tests/test_clarin_write.py \
91+
tests/test_models_clarin.py tests/test_clarin_usage_contract.py"
92+
if [ "${{ matrix.impl }}" = "dtq" ]; then
93+
python -m pytest $FILES -v # full CLARIN surface, incl. dtq_only
94+
else
95+
python -m pytest $FILES -v -m "not dtq_only" # shared contract only
96+
fi
97+
98+
# ---- Consumer smoke jobs -------------------------------------------------
99+
# Turn "the API surface is a superset" into "the consumers still import/run".
100+
# Gated on CONSUMER_READ_TOKEN: until that read-scoped token for the private
101+
# consumer repos exists, the gate job reports enabled=false and consumer-smoke
102+
# is skipped (a clean green), per plan §6.3.
103+
check-consumer-token:
104+
runs-on: ubuntu-latest
105+
outputs:
106+
enabled: ${{ steps.probe.outputs.enabled }}
107+
steps:
108+
- id: probe
109+
env:
110+
TOKEN: ${{ secrets.CONSUMER_READ_TOKEN }}
111+
run: echo "enabled=${{ env.TOKEN != '' }}" >> "$GITHUB_OUTPUT"
112+
113+
consumer-smoke:
114+
needs: check-consumer-token
115+
if: needs.check-consumer-token.outputs.enabled == 'true'
116+
runs-on: ubuntu-latest
117+
strategy:
118+
fail-fast: false
119+
matrix:
120+
include:
121+
- repo: DSpace-ISstag-integration
122+
ref: main
123+
smoke: python -m pytest tests/ mcp/tests/ -q
124+
- repo: dspace-rest-test
125+
ref: master
126+
smoke: python -c "import dspace_rest_client.client"
127+
- repo: dspace-import-clarin
128+
ref: main
129+
smoke: python -c "import dspace_rest_client.client"
130+
# dspace-item-importer is intentionally omitted until its .gitmodules
131+
# is repointed off the deleted `dtq-dev` branch (plan §6.3 / brief §5).
132+
steps:
133+
- uses: actions/checkout@v6
134+
with:
135+
path: candidate
136+
137+
- uses: actions/checkout@v6
138+
with:
139+
repository: dataquest-dev/${{ matrix.repo }}
140+
ref: ${{ matrix.ref }}
141+
token: ${{ secrets.CONSUMER_READ_TOKEN }}
142+
submodules: recursive
143+
path: consumer
144+
145+
- name: Set up Python
146+
uses: actions/setup-python@v6
147+
with:
148+
python-version: "3.10"
149+
150+
- name: Point the consumer submodule at this candidate commit
151+
run: |
152+
rm -rf consumer/libs/dspace-rest-python
153+
cp -r candidate consumer/libs/dspace-rest-python
154+
155+
- name: Install and smoke
156+
working-directory: consumer
157+
run: |
158+
python -m pip install --upgrade pip
159+
pip install ./libs/dspace-rest-python
160+
if [ -f requirements.lock ]; then pip install -r requirements.lock; fi
161+
if [ -f libs/dspace-rest-python/requirements-test.txt ]; then
162+
pip install -r libs/dspace-rest-python/requirements-test.txt
163+
fi
164+
${{ matrix.smoke }}

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,5 @@ __pypackages__/
1313
env/
1414
venv/
1515
.idea/
16+
.coverage
17+
coverage.xml

dspace_rest_client/client.py

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1096,7 +1096,12 @@ def get_owningCollection(self, item_uuid):
10961096
url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/owningCollection'
10971097
try:
10981098
r = self.api_get(url, None, None)
1099-
self.verify_response(r, f"item:{item_uuid}", True)
1099+
# On a non-200, verify_response records self._last_err and returns
1100+
# False - return None here (not an empty, truthy Collection) so the
1101+
# caller's `owning_col is None and last_err.status_code == 401`
1102+
# reauth path (src/repo/_audit.py) actually fires.
1103+
if not self.verify_response(r, f"item:{item_uuid}", True):
1104+
return None
11001105
r_json = parse_json(response=r)
11011106
return Collection(r_json)
11021107
except ValueError:
@@ -1257,7 +1262,12 @@ def create_submit_group(self, collection):
12571262
url = f'{self.API_ENDPOINT}/core/collections/{collection.uuid}/submittersGroup'
12581263
r = self.api_post(url, json={}, params=None)
12591264
if r.status_code == 201:
1260-
return Group(parse_json(r))
1265+
# a 201 with an empty/invalid body would make Group(None) here; return
1266+
# None instead so a caller's `if not group` guard fires cleanly rather
1267+
# than passing a uuid-less Group into add_member()
1268+
j = parse_json(r)
1269+
if j:
1270+
return Group(j)
12611271
return None
12621272

12631273
def add_member(self, group, eperson):
@@ -1350,7 +1360,9 @@ def get_items_from_collection(self, collection_id, page=0, size=1000):
13501360
items = list()
13511361
r = self.api_get(url)
13521362
r_json = parse_json(r)
1353-
if '_embedded' in r_json:
1363+
# a failed request parses to None; return the empty list rather than
1364+
# crashing on `'_embedded' in None`
1365+
if r_json and '_embedded' in r_json:
13541366
if 'searchResult' in r_json['_embedded']:
13551367
if '_embedded' in r_json['_embedded']['searchResult']:
13561368
for item_resource in r_json['_embedded']['searchResult']['_embedded']['objects']:
@@ -1367,7 +1379,9 @@ def get_bundle_by_name(self, name, item_uuid):
13671379
"""
13681380
url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/bundles'
13691381
r_json = self.fetch_resource(url, params=None)
1370-
if '_embedded' in r_json:
1382+
# fetch_resource returns None on any non-200 (records self._last_err);
1383+
# guard so a failed lookup is a clean None, not a NoneType subscript crash
1384+
if r_json and '_embedded' in r_json:
13711385
if 'bundles' in r_json['_embedded']:
13721386
for bundle in r_json['_embedded']['bundles']:
13731387
if bundle['name'] == name:
@@ -1381,9 +1395,13 @@ def get_resource_policy(self, bundle_uuid):
13811395
url = f'{self.API_ENDPOINT}/authz/resourcepolicies/search/resource?uuid={bundle_uuid}&embed=eperson&embed=group'
13821396
r = self.api_get(url)
13831397
r_json = parse_json(r)
1384-
if '_embedded' in r_json:
1385-
if 'resourcepolicies' in r_json['_embedded']:
1386-
return r_json['_embedded']['resourcepolicies'][0]
1398+
# guard against a failed request (r_json is None) and against an empty
1399+
# policy list - both must be a clean None, not a TypeError/IndexError
1400+
if r_json and '_embedded' in r_json and 'resourcepolicies' in r_json['_embedded']:
1401+
policies = r_json['_embedded']['resourcepolicies']
1402+
if policies:
1403+
return policies[0]
1404+
return None
13871405

13881406
def create_resource_policy(self, resource_uuid, data, group_uuid=None, eperson_uuid=None):
13891407
"""
@@ -1475,7 +1493,15 @@ def get_user_by_email(self, email):
14751493
params = {'email': email}
14761494
try:
14771495
response = self.api_get(url, params=params)
1496+
# a miss returns 404 (with a JSON error body): building a User from
1497+
# that yields a truthy, uuid-less object that passes `if user:` and
1498+
# fails much later. Treat any non-200 as "no such user" -> None.
1499+
if response.status_code != 200:
1500+
_logger.error(f"No user for email {email}: {response.status_code}")
1501+
return None
14781502
user_data = parse_json(response)
1503+
if not user_data:
1504+
return None
14791505
return User(user_data)
14801506
except Exception as e:
14811507
_logger.error(f"Error retrieving user by email {email}: {e}")

dspace_rest_client/models.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,7 @@ def __init__(self, api_resource=None):
407407
@param api_resource: API result object to use as initial data
408408
"""
409409
super().__init__(api_resource)
410+
api_resource = api_resource or {}
410411
self.type = 'group'
411412
if 'name' in api_resource:
412413
self.name = api_resource['name']
@@ -442,6 +443,7 @@ def __init__(self, api_resource=None):
442443
@param api_resource: API result object to use as initial data
443444
"""
444445
super().__init__(api_resource)
446+
api_resource = api_resource or {}
445447
self.type = 'user'
446448
if 'name' in api_resource:
447449
self.name = api_resource['name']

pytest.ini

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
[pytest]
2+
# Markers used to slice the suite for the branch-differential CI run
3+
# (see .github/workflows/tests.yml :: differential-contract). A pytest.ini
4+
# takes precedence over any [tool.pytest.ini_options] a future pyproject.toml
5+
# might add, so the marker registry stays in one place regardless of packaging.
6+
markers =
7+
dtq_only: behaviour introduced on dtq; not expected to hold on the main implementation
8+
clarin: exercises the CLARIN/UFAL surface (main-lineage consumers)
9+
dq: exercises the DQ integration surface (DSpace-ISstag-integration)

requirements-test.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,5 @@
55
pytest>=7.0
66
requests-mock>=1.11
77
requests
8+
# coverage floor is enforced in CI (tests.yml :: test job, --cov-fail-under)
9+
pytest-cov>=4.0

tests/_helpers.py

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@
3232
COLLECTION_UUID = "22222222-2222-2222-2222-222222222222"
3333
BITSTREAM_UUID = "9f54ef33-c454-4d8e-a5fe-79d8291045ba"
3434
ANON_GROUP_UUID = "6ecfd145-3b7d-429e-ab31-ef6905a05763"
35+
# Used by the CLARIN-side suites (eperson/group lookups, submit groups).
36+
EPERSON_UUID = "33333333-3333-3333-3333-333333333333"
37+
GROUP_UUID = "44444444-4444-4444-4444-444444444444"
38+
BUNDLE_UUID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"
3539

3640

3741
def make_client(api_endpoint: str = API) -> DSpaceClient:
@@ -112,3 +116,65 @@ def policy_json(pid: int = 1, action: str = "READ", group_name: str = "Anonymous
112116
if start_date is not None:
113117
d["startDate"] = start_date
114118
return d
119+
120+
121+
def raw_policy_json(pid: int = 1, action: str = "READ", **extra) -> dict:
122+
"""A resource policy in the *raw* shape the CLARIN ``get_resource_policy``
123+
returns (a plain dict the caller subscripts as ``["id"]``), not a model."""
124+
d = {"id": pid, "action": action, "type": "resourcepolicy"}
125+
d.update(extra)
126+
return d
127+
128+
129+
def group_json(uuid: str = GROUP_UUID, name: str = "Anonymous",
130+
permanent: bool = False, **extra) -> dict:
131+
d = {"uuid": uuid, "name": name, "type": "group", "permanent": permanent}
132+
d.update(extra)
133+
return d
134+
135+
136+
def user_json(uuid: str = EPERSON_UUID, email: str = "tester@dspace.test",
137+
name: str = "Tester", netid: str = None, can_login: bool = True,
138+
**extra) -> dict:
139+
d = {"uuid": uuid, "type": "eperson", "name": name, "email": email,
140+
"canLogIn": can_login}
141+
if netid is not None:
142+
d["netid"] = netid
143+
d.update(extra)
144+
return d
145+
146+
147+
def label_json(lid: int = 10, label: str = "PUB", title: str = "Publicly available",
148+
icon: str = "pub.png", extended: bool = False) -> dict:
149+
return {"id": lid, "label": label, "title": title, "icon": icon,
150+
"extended": extended}
151+
152+
153+
def license_json(lid: int = 1, name: str = "CC-BY",
154+
definition: str = "https://creativecommons.org/licenses/by/4.0/",
155+
confirmation: int = 1, required_info: str = "SEND_TOKEN",
156+
label: dict = None, extended: list = None) -> dict:
157+
d = {"id": lid, "name": name, "definition": definition,
158+
"confirmation": confirmation, "requiredInfo": required_info}
159+
if label is not None:
160+
d["clarinLicenseLabel"] = label
161+
if extended is not None:
162+
d["extendedClarinLicenseLabels"] = extended
163+
return d
164+
165+
166+
def clarin_allowance_json(aid: int = 1, **extra) -> dict:
167+
d = {"id": aid, "type": "clarinlruallowance"}
168+
d.update(extra)
169+
return d
170+
171+
172+
def search_envelope(items: list) -> dict:
173+
"""The ``discover/search/objects`` HAL envelope, wrapping each item as an
174+
``indexableObject``. Used by ``get_items_from_collection`` and
175+
``search_objects``.
176+
"""
177+
return {"_embedded": {"searchResult": {
178+
"page": {"totalElements": len(items)},
179+
"_embedded": {"objects": [
180+
{"_embedded": {"indexableObject": it}} for it in items]}}}}

0 commit comments

Comments
 (0)