improve python-client tests#50
Open
nebay-abraha wants to merge 13 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
nebay-abraha
force-pushed
the
test-codeql
branch
from
b2a1da2 to
e755928
Compare
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Python client’s testing and CI setup by separating unit vs. integration tests, updating pytest configuration/dependencies, and improving inventory/ELN integration test robustness (including bootstrapping an RSpace Docker instance in GitHub Actions).
Changes:
- Migrate pytest configuration into
pyproject.toml, add anintegrationmarker, and update dev/test dependencies (pytest, playwright, setuptools pin). - Make integration tests more robust by ensuring prerequisite data exists and by using safer temp-file handling.
- Add a GitHub Actions integration-test job that boots
rspace-dockerand generates an API key via Playwright automation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rspace_client/tests/sample_post_test.py | Adds unit tests for SamplePost location + barcode serialization behavior. |
| rspace_client/tests/invapi_test.py | Marks as integration and reduces brittleness of assertions; updates barcode test expectations. |
| rspace_client/tests/inv_lom_test.py | Marks LOM API test as integration. |
| rspace_client/tests/elnapi_test.py | Marks as integration; improves robustness via temp files and pre-creating test data. |
| rspace_client/inv/inv.py | Updates Barcode dataclass serialization and adds location support to SamplePost. |
| pytest.ini | Removes legacy pytest config (moved to pyproject.toml). |
| pyproject.toml | Updates Python baseline/deps and defines pytest options + integration marker. |
| DEVELOPING.md | Updates Python version guidance and clarifies unit vs. integration test commands. |
| .github/workflows/codeql-and-tests.yml | Adds unit-test matrix and new integration-test job with RSpace Docker bootstrapping. |
| .github/scripts/get_rspace_api_key.py | New Playwright-based script to generate an RSpace API key in CI. |
| .env.example | Adds username/password entries to support API key generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import pprint | ||
| import requests | ||
| from typing import Optional, Sequence, Union, List, TypedDict, BinaryIO | ||
| from typing import Optional, Sequence, Union, List, TypedDict, BinaryIO, ClassVar |
Comment on lines
+34
to
+40
| @dataclass | ||
| class Barcode: | ||
| data: str | ||
| format: BarcodeFormat | ||
| data: Optional[str] = None | ||
| format: Optional[BarcodeFormat] = None | ||
| description: str = "" | ||
| newBarcodeRequest: bool = True | ||
| id: Optional[str] = "" | ||
| new_barcode_request: bool = True | ||
| id: Optional[str] = None |
Comment on lines
+695
to
+701
| "SA12345", outfile="out10.png", barcode_type=inv.BarcodeFormat.QR | ||
| ) | ||
| self.assertEqual(293, len(qr_bytes)) | ||
| self.assertTrue(len(qr_bytes) > 0) | ||
| self.assertTrue(qr_bytes.startswith(b"\x89PNG\r\n\x1a\n")) | ||
| self.assertTrue(os.path.exists("out10.png")) | ||
| self.assertTrue(os.path.getsize("out10.png") > 0) | ||
| os.remove("out10.png") |
| runs-on: ubuntu-latest | ||
| runs-on: ubuntu-24.04 | ||
| timeout-minutes: 20 | ||
| permissions: |
Comment on lines
+154
to
+161
| - name: Generate RSpace API Key | ||
| id: generate_key | ||
| run: poetry run python .github/scripts/get_rspace_api_key.py | ||
| env: | ||
| RSPACE_URL: http://localhost:8080 | ||
| RSPACE_USERNAME: ${{ secrets.RSPACE_USERNAME }} | ||
| RSPACE_PASSWORD: ${{ secrets.RSPACE_PASSWORD }} | ||
|
|
| match = re.search(r"Key:\s*([A-Za-z0-9]{32})", info_text) | ||
| if not match: | ||
| raise RuntimeError( | ||
| f"API key regex did not match — text length {len(info_text)}, starts with: {repr(info_text[:20])}" |
| import os | ||
| import re | ||
| import sys | ||
| from playwright.sync_api import sync_playwright, expect |
- bump pytest and pin setuptools - add RSpace bootstrapping and python playwright api key generation - update development and test run guidance - python version baseline and clearer unit vs integration instructions - make test more robust - run integration tests against rspace-docker container - update barcode data class to align with rspace-java-client - add sample_post tests
…it rspace instance
…ct to init rspace instance" This reverts commit e3ab722. Replicating rspace-web's e2e.yml boot mechanism required a full Maven/Node rebuild of rspace-web on every CI run (jetty:run-war forks a full "package" phase regardless of -Dmaven.war.skip), which is far more expensive than the docker-compose flow it replaced. Reverting to the known-working setup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
this avoids constant failure due to ui changes which playwright ui depends on
…alidate the boolean response, and updated the related tests to match the corrected API behavior
Comment on lines
+694
to
+701
| qr_bytes = self.invapi.barcode( | ||
| "SA12345", outfile="out10.png", barcode_type=inv.Barcode.QR | ||
| "SA12345", outfile="out10.png", barcode_type=inv.BarcodeFormat.QR | ||
| ) | ||
| self.assertEqual(293, len(qr_bytes)) | ||
| self.assertTrue(len(qr_bytes) > 0) | ||
| self.assertTrue(qr_bytes.startswith(b"\x89PNG\r\n\x1a\n")) | ||
| self.assertTrue(os.path.exists("out10.png")) | ||
| self.assertTrue(os.path.getsize("out10.png") > 0) | ||
| os.remove("out10.png") |
Comment on lines
2753
to
2759
| try: | ||
| response = requests.get(url, headers=headers) | ||
| return response.status_code == 200 | ||
| if response.status_code != 200: | ||
| return False | ||
| return bool(response.json()) | ||
| except requests.exceptions.RequestException: | ||
| return False |
| id: rspace_web_release | ||
| run: | | ||
| set -euo pipefail | ||
| latest_tag=$(curl -fsSL https://api.github.com/repos/rspace-os/rspace-web/releases/latest | jq -r '.tag_name') |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.