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.

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

Replace vcr-py for tests #1198

Draft
wants to merge 39 commits into
base: master
Choose a base branch
from
Draft

Replace vcr-py for tests #1198

wants to merge 39 commits into from

Conversation

markbader
Copy link
Contributor

@markbader markbader commented Sep 26, 2024 •
edited
Loading

Description:

  • This PR should remove vcr-py and introduce a proxy server that replays API requests for the tests.

Todos:

Make sure to delete unnecessary points or to check all before merging:

  • Updated Changelog

Fixing tests:

  • Add annotation to local wk instance (e.g. tests/test_annotation.py::test_annotation_from_url)
  • Zarr Streaming doesn't seem to work for local wk instance(e.g. tests/test_annotation.py::test_bounding_box_roundtrip)
  • Remove date-strings from datasets generated within tests (e.g. tests/test_examples.py::test_upload_image_data)
  • Replaying responses for tests/dataset/test_dataset_add_remote_mag_and_layer.py does not work yet.

@markbader markbader self-assigned this Sep 26, 2024
@normanrz normanrz added this to the 2.x milestone Oct 9, 2024
@markbader
Merge branch 'master' into introduce-proxay-for-tests
a9ad224
@markbader
working on formatting and cleaning responses.
33eb8e1
@markbader
Merge branch 'master' into introduce-proxay-for-tests
72dca38
@markbader
Add type hint.
13437bc
@markbader
Update uv lock.
f0b1604
@markbader
Change API requests in tests to use local webknossos.
e4bda84
@markbader
Experiment with failing tests.
999e76e
@markbader
Merge branch 'master' into introduce-proxay-for-tests
760c7b0
@markbader
Remove download from pytest fixture.
2b4d5a4
@markbader
Merge branch 'master' into introduce-proxay-for-tests
594e62c
@markbader
Update test-cassettes and fix some tests.
1ff7de9
@markbader
Run linter.
b051312
@markbader
Merge branch 'master' into introduce-proxay-for-tests
6e67c64
@markbader
Remove time strings from tests.
a2b4c08
@markbader
Adapt tests with zarr streaming.
20e540b
@markbader
Merge branch 'master' into introduce-proxay-for-tests
b48e4fa
@markbader
Working on tests that use annotations.
031759e
@markbader
Add debugging test mode that starts replay proxy.
99f5df8
@markbader
Add fixture for aiohttp use env variables.
15d8af7
@markbader
Set l4_sample public for running the remote_mags tests.
6bce46d
@markbader
Add cassette files for tests.
a6fbe04
@markbader
Merge branch 'master' into introduce-proxay-for-tests
be074fa
@markbader
Merge branch 'master' into introduce-proxay-for-tests
248389a
@markbader
Move access to remote MagViews out of fixture to capture requests in …
78353a0 
…proxay.
@markbader
Run linter.
85cc153
@markbader
Update cassette for annotation_upload_download_roundtrip.
53ae8f3
normanrz
normanrz reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

Looks already pretty good. Maybe you can comment on the state of the commented-out code. I think some of that could be revived in a follow-up.

webknossos/test.sh Show resolved Hide resolved
webknossos/testdata/l4_sample_snipped/color/1/header.wkw Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why not download from static.webknossos.org in the CI?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, we could add it in the local_wk_setup.sh to ensure that the tests are running local and in the CI.

webknossos/tests/conftest.py Show resolved Hide resolved
webknossos/tests/dataset/test_dataset.py Show resolved Hide resolved
webknossos/webknossos/client/api_client/errors.py Show resolved Hide resolved
webknossos/webknossos/dataset/dataset.py Show resolved Hide resolved
webknossos/webknossos/dataset/layer.py Show resolved Hide resolved
webknossos/webknossos/dataset/mag_view.py Show resolved Hide resolved
webknossos/tests/test_annotation.py
assert segment_info.anchor_position == (2785, 4423, 1792)
segment_info.name = "Test Segment"
segment_info.color = (1, 0, 0, 1)
# segment_info = annotation.get_volume_layer_segments("Volume")[2504698]
Copy link
Member

Choose a reason for hiding this comment

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

Can we bring this back?

webknossos/tests/dataset/test_dataset_download_upload_remote.py
"https://webknossos.org/links/93zLg9U9vJ3c_UWp",
"http://localhost:9000/datasets/Organization_X/l4_sample",
"http://localhost:9000/datasets/Organization_X/l4_sample/view",
# "http://localhost:9000/datasets/Organization_X/l4_sample_dev_sharing/view?token=ilDXmfQa2G8e719vb1U9YQ#%7B%22orthogonal%7D",
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to also test sharing links and short links. Can be a followup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normanrz normanrz normanrz left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@markbader markbader

Labels
None yet
Projects
None yet
Milestone
2.x
Development

Successfully merging this pull request may close these issues.
2 participants
@markbader @normanrz