System test framework #392
Conversation
dockerfiles/test-system/Dockerfile
Outdated
| RUN pip install --upgrade pytest | ||
| RUN cd /tmp/mlrun && python -m pip install --no-cache-dir ".[api]" && mv tests /tests && cd /tmp && rm -rf mlrun | ||
|
|
||
| CMD ["pytest", "-rf", "-v", "--disable-warnings", "/tests/system"] |
There was a problem hiding this comment.
newline eof
also in some other places
|
|
||
| COPY . /tmp/mlrun | ||
| RUN pip install --upgrade pytest | ||
| RUN cd /tmp/mlrun && python -m pip install --no-cache-dir ".[api]" && mv tests /tests && cd /tmp && rm -rf mlrun |
There was a problem hiding this comment.
- Why aren't you copying tests to
/testsin the first place (after line 18COPY tests /tests) - What is the significance of removing /tmp/mlrun/mlrun? (add comment if needed)
There was a problem hiding this comment.
We're removing mlrun to ensure that the testing environment is as close to a user environment as possible. As in the relevant code is in site-packages and nothing else
| logger = create_logger(level='debug', name='test') | ||
|
|
||
|
|
||
| class TestMLRunSystem: |
There was a problem hiding this comment.
| class TestMLRunSystem: | |
| class MLRunSystemTest: |
| not configured, | ||
| reason=f'This is a system test, add the needed environment variables {*cls.mandatory_env_vars,} ' | ||
| 'in tests/system/env.yml to run it', | ||
| )(test) |
There was a problem hiding this comment.
misconfiguration should cause the tests to fail not be skipped, no?
That way we will get false positives without anything running
There was a problem hiding this comment.
That way, CI will fail 100% of the time right now that there isn't a constant test system
| 'V3IO_API', | ||
| 'V3IO_USERNAME', | ||
| 'V3IO_ACCESS_KEY', | ||
| ] |
There was a problem hiding this comment.
Why are these class properties (and handled via classmethods) and not simple data fields on the class instance (defined in __init__)?
There was a problem hiding this comment.
These are indeed static properties that relate to every object of the class, so I'm using them as intended
| V3IO_PASSWORD: | ||
|
|
||
| # access key used to add v3io mounts to pods - e.g. 4c1c2011-618f-434a-abd6-d456770fd33c | ||
| V3IO_ACCESS_KEY: |
There was a problem hiding this comment.
Suggest to put non empty placeholders - style "REPLACEME" - this is always a good idea to not have things the user needs to input default to empty string. something like this can easily go unnoticed in outputs / logs / error messages.
Something meaningful will catch the eye and be clear
| @@ -0,0 +1,14 @@ | |||
| # The mlrun-api URL. e.g. https://mlrun-api.default-tenant.app.hedingber-28-1.iguazio-cd2.com | |||
There was a problem hiding this comment.
Why is this a yml file ? env key=value shouldn't be declared verbatim in a yaml (https://docs.docker.com/compose/env-file/)
|
|
||
| kfp.compiler.Compiler().compile(dask_pipe, 'daskpipe.yaml', type_check=False) | ||
| arguments = {'x': 4, 'y': -5} | ||
| artifact_path = '/User/test' |
| run['spec'], | ||
| parameters={'x': 4, 'y': -5}, | ||
| outputs=['run_id'], | ||
| output_path='/User/test', |
Added a system test framework.
TestMLRunSystemclass to extend.env.ymlfile to specify the address and credentials.@TestMLRunSystem.skip_test_env_not_configured) is put above each system test class in order for the tests to be skipped if theenv.ymlisn't configured properly.make test-systemcreates a docker container that only doespip install mlrun[api] pytestbefore running the system tests, in order to run the system tests from a user's perspective.