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

♻️ Is638/dynamic sidecar refactors tests (round 3) #3154

Merged
merged 24 commits into from
Jun 29, 2022

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 28, 2022

Follows from PR #3152 (when you review this notice that, if the former PR is still not merged into master, the list of Files changed will also include its files).

What do these changes do?

The main goal of the PR is to simplify the test infrastructure, in particular the fixtures that patch/mock env vars and functionality.

  • ♻️ test refactoring strategy (see comments in code):
    • env vars are used to configure application
    • mock_environment defines a base app config suitable for unit-tests
    • overriding this configuration -> produces a different app fixture
  • Other changes:
    • AppState is a decorator class helper
    • ♻️ replaced DY_VOLUME global constant. Now in app.settings captured from DY_VOLUME env (i.e. syncs with Dockerfile)
    • 🔨 scripts/Makefile recipe to display settings: make info

Related issue/s

How to test

CI

Checklist

  • Openapi changes? make openapi-specs, git commit ... and then make version-*)
  • Database migration script? cd packages/postgres-database, make setup-commit, sc-pg review -m "my changes"
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@pcrespov pcrespov changed the title ♻️ Is638/dynamic sidecar refactors tests (round 3) WIP: ♻️ Is638/dynamic sidecar refactors tests (round 3) Jun 28, 2022
@pcrespov pcrespov self-assigned this Jun 28, 2022
@pcrespov pcrespov added the a:dynamic-sidecar dynamic-sidecar service label Jun 28, 2022
@pcrespov pcrespov added this to the Diolkos milestone Jun 28, 2022
@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #3154 (0750896) into master (89cf160) will decrease coverage by 0.0%.
The diff coverage is 92.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3154     +/-   ##
========================================
- Coverage    81.1%   81.0%   -0.1%     
========================================
  Files         711     711             
  Lines       30630   30646     +16     
  Branches     3948    3949      +1     
========================================
+ Hits        24844   24853      +9     
- Misses       4948    4951      +3     
- Partials      838     842      +4     
Flag Coverage Δ
integrationtests 66.2% <ø> (+<0.1%) ⬆️
unittests 77.4% <92.5%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...sidecar/src/simcore_service_dynamic_sidecar/cli.py 0.0% <0.0%> (ø)
...ice_dynamic_sidecar/models/domains/shared_store.py 100.0% <ø> (ø)
...namic_sidecar/models/schemas/application_health.py 100.0% <ø> (ø)
...imcore_service_dynamic_sidecar/core/application.py 95.8% <100.0%> (-0.7%) ⬇️
...c/simcore_service_dynamic_sidecar/core/settings.py 94.2% <100.0%> (-3.9%) ⬇️
...core_service_dynamic_sidecar/modules/mounted_fs.py 100.0% <100.0%> (ø)
...ore_service_director_v2/utils/client_decorators.py 73.3% <0.0%> (-3.4%) ⬇️
...imcore_service_dynamic_sidecar/core/docker_logs.py 87.5% <0.0%> (-3.2%) ⬇️
...rector_v2/modules/comp_scheduler/base_scheduler.py 86.7% <0.0%> (-1.9%) ⬇️
... and 11 more

@pcrespov pcrespov changed the title WIP: ♻️ Is638/dynamic sidecar refactors tests (round 3) ♻️ Is638/dynamic sidecar refactors tests (round 3) Jun 28, 2022
@pcrespov pcrespov marked this pull request as ready for review June 28, 2022 21:52
@@ -53,7 +67,9 @@ def compose_spec(dynamic_sidecar_network_name: str) -> str:
"services": {
"first-box": {
"image": "busybox",
"networks": [dynamic_sidecar_network_name],
"networks": [
dynamic_sidecar_network_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the first time I see it, but what is the advantage in adding this comma?

Copy link
Member Author

@pcrespov pcrespov Jun 29, 2022

Choose a reason for hiding this comment

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

It is a visual aid. I add it to enforce black to create multiline so the json-like structure looks like yaml. That would be

services:
    first-box:
        image: busybox
        networks:
           - dynamic_sidecar_ntework_name

It helps me seeing better all sections since we are used to yaml more than json for the compose specs.
It is a small thing that I can also reverse

@pcrespov pcrespov force-pushed the is638/dy-sidecar-testfixtures branch from f75d152 to 1e4e3a1 Compare June 29, 2022 06:39
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.

great! thanks!

services/dynamic-sidecar/tests/unit/conftest.py Outdated Show resolved Hide resolved
services/dynamic-sidecar/tests/unit/conftest.py Outdated Show resolved Hide resolved
@pcrespov pcrespov requested a review from GitHK June 29, 2022 07:01
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.4% 0.4% Duplication

@pcrespov pcrespov merged commit f17b96b into ITISFoundation:master Jun 29, 2022
@pcrespov pcrespov deleted the is638/dy-sidecar-testfixtures branch June 29, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:dynamic-sidecar dynamic-sidecar service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants