Skip to content

♻️ Maintenance: Unify ApplicationSettings Testing Across Services and Prepare for External Env File Support #7919

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Jun 22, 2025
Merged

♻️ Maintenance: Unify ApplicationSettings Testing Across Services and Prepare for External Env File Support #7919

merged 19 commits into from
Jun 22, 2025

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 17, 2025 •
edited
Loading

What do these changes do?

This PR aims to standardize the testing of ApplicationSettings across all services. Currently, there are inconsistencies in how environment variables are mocked in tests. These differences must be addressed before we can enforce the upcoming feature that allows replacing .env-devel with an external env file. Some groundwork was left as-is where already present, but a thorough review is still required.

Highlights

  • All services now include a test_core_settings.py module
    • Exception: web/server, where the file is named test_application_settings.py image
  • Each test suite includes at least one test verifying ApplicationSettings behavior for different app_environment values
    • ⚠️ The fixtures used to mock environment variables differ across services (see Follow Up)

Follow-Up Work Required

To complete the unification:

  • ✅ Standardize fixture names used for mocking environment variables
  • ✅ Harmonize mocking workflows, which currently vary:
    • Some use .env-devel filtered via docker-compose.yml’s environment section
    • Others rely on different or inconsistent methods
  • Unify usage of external_envfile_dict: will allow to run tests against configurations in deploys
    • Refactor so that it has to be explicitly enabled in the test that we want to use it, instead of as a generic fixture (which might have unexpected and bad consequences)

Related issue/s

How to test

⚠️ Warning: Use --external-envfile with care. Some tests might unintentionally affect real deployments (e.g. delete/write databases, ...)

This is how we validate an ApplicationSettings against an actual envfile ( NOTE: only for services that use --external-envfile )

  1. create a symbolic link to the repo root and name it .secrets
cd osparc-simcore
ln -s /path/to/osparc-config/deployments/mydeploy.com/repo.config .secrets
  1. Update .vscode/launch.json from the template
  2. Run pytest with --external-envfile option
    • UI: Open a test_core_settings.py and run Python: Test w/ repo.config
      image
    • Command line (e.g. for catalog)
cd services/catalog
# w/o external
pytest -vv -s tests/unit -k test_valid_application_settings

# w/ external envfile
pytest -vv --log-cli-level=INFO --external-envfile=../../.secrets tests/unit -k test_valid_application_settings```

image

Dev-ops

None

@codecov
Copy link

codecov bot commented Jun 17, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.05%. Comparing base (6903354) to head (ef422ad).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7919      +/-   ##
==========================================
+ Coverage   88.04%   88.05%   +0.01%     
==========================================
  Files        1846     1839       -7     
  Lines       71225    71035     -190     
  Branches     1220     1220              
==========================================
- Hits        62707    62553     -154     
+ Misses       8166     8130      -36     
  Partials      352      352
Flag Coverage Δ
integrationtests 64.20% <ø> (-0.08%) ⬇️
unittests 86.65% <ø> (+0.02%) ⬆️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library 93.92% <ø> (ø)
pkg_dask_task_models_library 79.62% <ø> (ø)
pkg_models_library 93.27% <ø> (ø)
pkg_notifications_library 85.26% <ø> (ø)
pkg_postgres_database 88.18% <ø> (ø)
pkg_service_integration 69.92% <ø> (ø)
pkg_service_library 72.67% <ø> (ø)
pkg_settings_library 90.90% <ø> (ø)
pkg_simcore_sdk 85.05% <ø> (ø)
agent 96.29% <ø> (ø)
api_server 92.31% <ø> (ø)
autoscaling 96.03% <ø> (ø)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.79% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.73% <ø> (ø)
director_v2 91.14% <ø> (-0.02%) ⬇️
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.09% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.00% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 89.00% <ø> (ø)
storage 87.53% <ø> (+0.07%) ⬆️
webclient ∅ <ø> (∅)
webserver 87.66% <ø> (-0.05%) ⬇️

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 6903354...ef422ad. 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 self-assigned this Jun 17, 2025
@pcrespov pcrespov added this to the Engage milestone Jun 17, 2025
@pcrespov pcrespov added a:storage issue related to storage service a:director issue related with the director service a:webserver webserver's codebase. Assigning the area is particularly useful for bugs a:catalog catalog service a:director-v2 issue related with the director-v2 service a:apiserver api-server service a:autoscaling autoscaling service in simcore's stack a:agent agent service a:invitations invitations service a:resource-usage-tracker resource usage tracker service a:clusters-keeper a:payments area: payments service a:dynamic-scheduler a:datcore-adapter a:notifications labels Jun 17, 2025
@pcrespov pcrespov force-pushed the is7395/test-settings branch from 94d77b8 to b04258d Compare June 17, 2025 18:40
@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Jun 17, 2025
@pcrespov pcrespov changed the title Is7395/test settings ♻️ Maintenance: Unify ApplicationSettings Testing Across Services and Prepare for External Env File Support Jun 17, 2025
@pcrespov pcrespov marked this pull request as ready for review June 17, 2025 18:45
@pcrespov pcrespov enabled auto-merge (squash) June 17, 2025 18:46
@pcrespov
Copy link
Member Author

pcrespov commented Jun 18, 2025

@mergify queue

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

mergify bot commented Jun 18, 2025 •
edited
Loading

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images

sanderegg
sanderegg approved these changes Jun 18, 2025
View reviewed changes
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

This can be a very DANGEROUS feature and I would like to be sure that it is clear with every dev that this can affect the deployments depending on what test(s) are being run. Otherwise I like that it is made simpler. but ideally we should prevent tests that are not only read-only...

pcrespov added 18 commits June 19, 2025 11:27
@pcrespov pcrespov force-pushed the is7395/test-settings branch from d588370 to 31b5d61 Compare June 19, 2025 09:40
@pcrespov pcrespov mentioned this pull request Jun 19, 2025
Open
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 22, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@pcrespov pcrespov merged commit c1d1065 into ITISFoundation:master Jun 22, 2025
95 checks passed
@pcrespov pcrespov deleted the is7395/test-settings branch June 25, 2025 08:18
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
Closed
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@giancarloromeo giancarloromeo giancarloromeo approved these changes

@GitHK GitHK GitHK approved these changes

@sanderegg sanderegg sanderegg approved these changes

@matusdrobuliak66 matusdrobuliak66 matusdrobuliak66 approved these changes

@bisgaard-itis bisgaard-itis Awaiting requested review from bisgaard-itis bisgaard-itis is a code owner

@wvangeit wvangeit Awaiting requested review from wvangeit wvangeit is a code owner

Assignees

@pcrespov pcrespov

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:agent agent service a:apiserver api-server service a:autoscaling autoscaling service in simcore's stack a:catalog catalog service a:clusters-keeper a:datcore-adapter a:director issue related with the director service a:director-v2 issue related with the director-v2 service a:dynamic-scheduler a:invitations invitations service a:notifications a:payments area: payments service a:resource-usage-tracker resource usage tracker service a:storage issue related to storage service a:webserver webserver's codebase. Assigning the area is particularly useful for bugs t:maintenance Some planned maintenance work

Projects

None yet

Milestone

Engage

Development

Successfully merging this pull request may close these issues.

test settings against repo.config(s) @pcrespov

5 participants

@pcrespov @giancarloromeo @GitHK @sanderegg @matusdrobuliak66