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

Full tests on macos #1788

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Full tests on macos #1788

merged 3 commits into from
Jul 8, 2024

Conversation

ikolomi
Copy link
Collaborator

@ikolomi ikolomi commented Jul 3, 2024

  1. Enabled all tests on macos latest with all engine versions.
  2. Changed pubsub tests to be run in separate task.
  3. Fixed uds message parsing in glide-core.
  4. Refactored pubsub tests

@ikolomi ikolomi requested a review from a team as a code owner July 3, 2024 15:53
Yury-Fridlyand

This comment was marked as resolved.

acarbonetto

This comment was marked as outdated.

@ikolomi

This comment was marked as outdated.

@Yury-Fridlyand Yury-Fridlyand marked this pull request as draft July 4, 2024 20:18
@Yury-Fridlyand

This comment was marked as outdated.

@ikolomi ikolomi force-pushed the full_tests_on_macos_rebased branch 9 times, most recently from 37d48b7 to 829502c Compare July 8, 2024 09:33
@ikolomi ikolomi marked this pull request as ready for review July 8, 2024 09:38
@ikolomi ikolomi requested a review from barshaul July 8, 2024 09:41
@ikolomi ikolomi force-pushed the full_tests_on_macos_rebased branch 2 times, most recently from c777180 to 43b1fcc Compare July 8, 2024 09:47
@ikolomi ikolomi requested a review from avifenesh July 8, 2024 09:48
@ikolomi ikolomi changed the title Full tests on macos rebased Full tests on macos Jul 8, 2024
Comment on lines 156 to 160
- name: Install dependencies
working-directory: ./python
run: |
python -m pip install --upgrade pip
pip install mypy-protobuf
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need this, you don't test mypy in this job

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, removed

…nged pubsub tests to be run in separate task. 3. Fixed uds message parsing in glide-core. 4. Refactored pubsub tests
@ikolomi ikolomi force-pushed the full_tests_on_macos_rebased branch from c423b4e to 3443307 Compare July 8, 2024 10:16
@ikolomi ikolomi requested a review from barshaul July 8, 2024 10:30
Copy link
Collaborator

@barshaul barshaul left a comment

Choose a reason for hiding this comment

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

Please cleanup the test file from all commented out lines and then i'll re-review

run: |
source .env/bin/activate
cd python/tests/
pytest -c ../../pytest_pubsub.ini --asyncio-mode=auto test_pubsub.py::TestPubSub
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove the pytest_pubsub.ini file and just do here:

pytest --asyncio-mode=auto -k test_pubsub

without any changes to the pytest.ini file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, done

run: |
source .env/bin/activate
cd python/tests/
pytest -c ../../pytest_pubsub.ini --asyncio-mode=auto test_pubsub.py::TestPubSub
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

match self.rotating_buffer.get_requests() {
Ok(requests) => {
if !requests.is_empty() {
// continue to read from socket
Copy link
Collaborator

Choose a reason for hiding this comment

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

the comment isn't in the right place, should be above 126

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct, fixed

python/pytest.ini Show resolved Hide resolved
python/python/tests/test_pubsub.py Show resolved Hide resolved
Comment on lines 604 to 608
# if cluster_mode:
# # Since all tests run on the same cluster, when closing the client, garbage collector can be called after another test will start running
# # In cluster mode, we check how many subscriptions received the message
# # So to avoid flakiness, we make sure to unsubscribe from the channels
# await listening_client.custom_command(["SUNSUBSCRIBE", channel])
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remoived

Comment on lines 663 to 671

# await cast(GlideClusterClient, publishing_client).publish(
# message, channel, sharded=True
# )

# await cast(GlideClusterClient, publishing_client).publish(
# message2, channel, sharded=True
# )

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

Comment on lines 697 to 701
# if cluster_mode:
# # Since all tests run on the same cluster, when closing the client, garbage collector can be called after another test will start running
# # In cluster mode, we check how many subscriptions received the message
# # So to avoid flakiness, we make sure to unsubscribe from the channels
# await listening_client.custom_command(["SUNSUBSCRIBE", channel])
Copy link
Collaborator

Choose a reason for hiding this comment

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

remvove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

# Publish messages to each channel
for channel, message in channels_and_messages.items():
# TODO: enable when client closing works
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

Comment on lines 772 to 775
# await cast(GlideClusterClient, publishing_client).publish(
# message, channel, sharded=True
# )

Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@ikolomi ikolomi force-pushed the full_tests_on_macos_rebased branch from 0a1c115 to e148bc5 Compare July 8, 2024 11:28
@ikolomi
Copy link
Collaborator Author

ikolomi commented Jul 8, 2024

@barshaul code cleaned up

@ikolomi ikolomi force-pushed the full_tests_on_macos_rebased branch from b8b5613 to 97ec251 Compare July 8, 2024 11:49
@ikolomi ikolomi force-pushed the full_tests_on_macos_rebased branch from 1fb8ff4 to 019edc8 Compare July 8, 2024 12:08
# Publish messages to each channel
for channel, message in channels_and_messages.items():
result = await publishing_client.publish(message, channel)
# TODO: enable when client closing works
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Collaborator

@barshaul barshaul left a comment

Choose a reason for hiding this comment

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

some leftovers, other than that LGTM

@ikolomi ikolomi force-pushed the full_tests_on_macos_rebased branch from 65544cd to b435c50 Compare July 8, 2024 12:35
@ikolomi ikolomi merged commit 0bf25d1 into main Jul 8, 2024
104 checks passed
@barshaul barshaul deleted the full_tests_on_macos_rebased branch July 8, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants