Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Oct 7, 2025

What do these changes do?

  1. Fixes wrong fixtures in tests
=========================== short test summary info ============================
ERROR tests/unit/pact_broker/test_pact_checkout_release.py::test_provider_against_pact - AttributeError: <module 'simcore_service_api_server.services_rpc.wb_api_server' from '/home/runner/work/osparc-simcore/osparc-simcore/.venv/lib/python3.11/site-packages/simcore_service_api_server/services_rpc/wb_api_server.py'> does not have the attribute '_checkout_licensed_item_for_wallet'
============================== 2 errors in 2.65s ===============================
  1. Improves skip test mechanism
  2. Improves doc
$ make help       

Targets for 'api-server':

reqs                 compiles pip requirements (.in -> .txt)
test-api             Runs schemathesis against development server (NOTE: make up-devel first)
test-pacts           Test pacts. Usage: PACT_BROKER_USERNAME=your_username PACT_BROKER_PASSWORD=your_password PACT_BROKER_URL=your_broker_url make test-pacts

In detail (AI)

This pull request refactors and improves the pact broker test setup for the API server, focusing on clearer test logic, more robust fixture management, and better handling of credentials and dependencies. The changes streamline how dependencies are overridden, improve the handling of missing credentials, and update test naming for clarity.

Test infrastructure improvements:

  • Updated the pact_broker_credentials fixture to use pytest.skip instead of pytest.fail when credentials are missing, and clarified its return type. This ensures tests are skipped gracefully if required environment variables or CLI arguments are not provided.
  • Changed dependency overrides in the running_test_server_url fixture to use an inline function for mocking identity, and ensured cleanup by removing the override after the test completes. [1] [2]

Test logic and naming:

  • Renamed test functions from test_provider_against_pact to more descriptive names (test_osparc_api_server_checkout_release_pact and test_osparc_api_server_licensed_items_pact), and removed unnecessary environment variable checks for test skipping, relying instead on the improved fixture logic. [1] [2]
  • Updated comments in test setup to clarify which pact contracts are being verified. [1] [2]

Fixture and import clean-up:

  • Improved the way dependencies are mocked in test fixtures for webserver_rpc and resource_usage_tracker, using more robust patching and dependency overrides, and cleaning up imports for clarity and correctness. [1] [2]
  • Added missing type annotations and removed unused imports in several test files for better code hygiene. [1] [2] [3] [4]

Documentation updates:

  • Improved the README.md instructions for pact broker usage, adding clearer steps for installing and publishing contracts.

Related issue/s

How to test

cd services/api-server             
(.venv) ➜  api-server git:(fix/pact-tests) ✗ make install-dev
(.venv) ➜  api-server git:(fix/pact-tests) ✗ make help       
Targets for 'api-server':

reqs                 compiles pip requirements (.in -> .txt)
test-api             Runs schemathesis against development server (NOTE: make up-devel first)
test-pacts           Test pacts. Usage: PACT_BROKER_USERNAME=your_username PACT_BROKER_PASSWORD=your_password PACT_BROKER_URL=your_broker_url make test-pacts
...
(.venv) ➜  api-server git:(fix/pact-tests) ✗ make test-pacts

Dev-ops

None

@pcrespov pcrespov changed the title ✅ [Maintenance ✅ [Maintenance] Fixes api-server/tests/unit/pact_broker testing Oct 7, 2025
@pcrespov pcrespov self-assigned this Oct 7, 2025
@pcrespov pcrespov added t:maintenance Some planned maintenance work a:apiserver api-server service labels Oct 7, 2025
@codecov
Copy link

codecov bot commented Oct 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.05%. Comparing base (83bfca1) to head (feb6a7f).
⚠️ Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (83bfca1) and HEAD (feb6a7f). Click for more details.

HEAD has 31 uploads less than BASE
Flag BASE (83bfca1) HEAD (feb6a7f)
unittests 32 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8473       +/-   ##
===========================================
- Coverage   87.64%   69.05%   -18.60%     
===========================================
  Files        1983      889     -1094     
  Lines       77279    39739    -37540     
  Branches     1333      175     -1158     
===========================================
- Hits        67729    27440    -40289     
- Misses       9151    12242     +3091     
+ Partials      399       57      -342     
Flag Coverage Δ
integrationtests 64.10% <ø> (-0.01%) ⬇️
unittests 91.78% <ø> (+5.44%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 76.75% <ø> (-8.21%) ⬇️
agent ∅ <ø> (∅)
api_server 91.78% <ø> (ø)
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 78.15% <ø> (-12.76%) ⬇️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 81.87% <ø> (-8.56%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 59.05% <ø> (-28.35%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83bfca1...feb6a7f. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov marked this pull request as ready for review October 7, 2025 14:38
@pcrespov pcrespov enabled auto-merge (squash) October 7, 2025 14:39
@pcrespov pcrespov added this to the Cheops milestone Oct 7, 2025
Copy link
Contributor

@wvangeit wvangeit left a comment

Choose a reason for hiding this comment

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

Thanks

@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Oct 7, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes failing pact broker tests in the API server by correcting test fixtures, improving skip mechanisms, and enhancing documentation. The tests were failing due to incorrect attribute references and dependency override issues.

Key changes:

  • Fixed test fixtures and dependency overrides that were causing AttributeError failures
  • Replaced pytest.fail with pytest.skip for better handling of missing credentials
  • Renamed test functions for clarity and improved documentation

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test_pact_licensed_items.py Removed unused import and improved test naming
test_pact_checkout_release.py Fixed dependency overrides and improved fixture cleanup
conftest.py Enhanced credential handling and dependency override management
README.md Improved documentation with clearer instructions
Makefile Streamlined test command documentation
tests/unit/conftest.py Added type annotations to improve code clarity

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@wvangeit wvangeit left a comment

Choose a reason for hiding this comment

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

Thx

Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements

@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2025

🧪 CI Insights

Here's what we observed from your CI run for feb6a7f.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI system-tests Base branch is broken, but retries were needed. Could be early signs of flakiness 👀 Broken 1 View View

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 7, 2025

@pcrespov pcrespov merged commit 77faa41 into ITISFoundation:master Oct 7, 2025
142 of 148 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:apiserver api-server service t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants