pydantic2 migration made integration tests green #6719
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## pydantic_v2_migration_do_not_squash_updates #6719 +/- ##
===============================================================================
+ Coverage 81.15% 82.58% +1.43%
===============================================================================
Files 1531 1524 -7
Lines 63884 63980 +96
Branches 2229 2229
===============================================================================
+ Hits 51846 52840 +994
+ Misses 11705 10804 -901
- Partials 333 336 +3
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
GitHK
changed the title
simcore-sdk and dynamic-sidecar
…quash_updates' into pr-osparc-pydantic2-integration-libraries2
GitHK
changed the title
simcore-sdk and dynamic-sidecar
packages/pytest-simcore/src/pytest_simcore/simcore_storage_service.py
Outdated
Show resolved
Hide resolved
services/dynamic-sidecar/tests/integration/test_modules_long_running_tasks.py
Outdated
Show resolved
Hide resolved
services/web/server/src/simcore_service_webserver/exporter/_formatter/xlsx/code_description.py
Show resolved
Hide resolved
…om:GitHK/osparc-simcore-forked into pr-osparc-pydantic2-integration-libraries2
…om:GitHK/osparc-simcore-forked into pr-osparc-pydantic2-integration-libraries2
…quash_updates' into pr-osparc-pydantic2-integration-libraries2
GitHK
changed the title
|
| @@ -205,7 +205,10 @@ class ServiceUpdate(ServiceMetaDataEditable, ServiceAccessRights): | |||
| class ServiceGet( | |||
| ServiceMetaDataPublished, ServiceAccessRights, ServiceMetaDataEditable | |||
| ): # pylint: disable=too-many-ancestors | |||
| owner: LowerCaseEmailStr | None | |||
| owner: LowerCaseEmailStr | None = Field( | |||
| None, | |||
There was a problem hiding this comment.
this change seems wrong. the ServiceGet comes from the catalog. this changes the API are you sure?
There was a problem hiding this comment.
I know we have the following scenario:
-> push service to osparc registry who's email is not part of the users
-> this owner filed is set to None
This should cover this case where. Since an integration test was failing this helped with fixing it. I'm not 100% sure if it might break anything though.
There was a problem hiding this comment.
Yes, the intent of the schema is that owner field is nullable but REQUIRED, here you made it NOT required.
There was a problem hiding this comment.
this is the API of the catalog so I guess this should not be changed
| @@ -261,7 +261,7 @@ def jupyter_service(docker_registry: str, node_meta_schema: dict) -> dict[str, A | |||
| ) | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="session", params=["2.0.4"]) | |||
| @pytest.fixture(scope="session", params=["2.0.7"]) | |||
There was a problem hiding this comment.
question: should this not go into upstream master?
There was a problem hiding this comment.
no we don't use it there for anything. It's OK like this, since this is used for the CI.
| | list[Any] | ||
| | dict[str, Any] | ||
| | None, | ||
| Field(union_mode="left_to_right"), |
There was a problem hiding this comment.
the annotation is interesting :-)
| @@ -205,7 +205,10 @@ class ServiceUpdate(ServiceMetaDataEditable, ServiceAccessRights): | |||
| class ServiceGet( | |||
| ServiceMetaDataPublished, ServiceAccessRights, ServiceMetaDataEditable | |||
| ): # pylint: disable=too-many-ancestors | |||
| owner: LowerCaseEmailStr | None | |||
| owner: LowerCaseEmailStr | None = Field( | |||
| None, | |||
There was a problem hiding this comment.
Yes, the intent of the schema is that owner field is nullable but REQUIRED, here you made it NOT required.
| @@ -94,13 +94,37 @@ def testing_environ_vars(env_devel_file: Path) -> EnvVarsDict: | |||
| # ensure we do not use the bucket of simcore or so | |||
| env_devel["S3_BUCKET_NAME"] = "pytestbucket" | |||
|
|
|||
| # ensure OpenTelemetry is not enabled | |||
| env_devel |= { | |||
| x: "null" | |||
There was a problem hiding this comment.
x? are you Ellon's supporter? env_name or e or n at least means something ... otherwise _ ;-)
| env_devel |= { | ||
| x: "null" | ||
| for x in ( | ||
| "AGENT_TRACING", |
There was a problem hiding this comment.
So what is you stating here is that by default all tracing are always disabled. Fine.
Cons: when we have a new service we will most probably forget to disable it.
Other approaches would be:
.env-develdisables tracing by default (bad if we want to us it locally)- Every services explicitly in his
conftest.pyoverriding it in theapp_environment(my preferred one)
What do these changes do?
Bonus:
Related issue/s
dy-static-file-serverservices osparc-services#157How to test
Dev-ops checklist