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

Add e2e tests for async pipe_family #1231

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

jernejfrank
Copy link
Contributor

Addtion to #1223 -- Converted the initial test script to integration tests.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 28c9d14 in 54 seconds

More details
  • Looked at 154 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. tests/function_modifiers/test_macros.py:1158
  • Draft comment:
    The test function is correctly marked with @pytest.mark.asyncio for async operations. Ensure that all async test functions are marked similarly.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The test functions in test_macros.py are using async_driver.Builder() to build the driver, which is correct for async operations. However, the execute method is being awaited, which is necessary for async functions. The test functions are correctly marked with @pytest.mark.asyncio, which is required for running async tests with pytest.
2. tests/resources/mutate_async.py:12
  • Draft comment:
    The use of await asyncio.sleep(0.0001) is appropriate for simulating async behavior in tests. Ensure this pattern is consistently used for async functions.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The mutate_async.py file defines async functions with the mutate decorator, which is appropriate for async operations. The use of await asyncio.sleep(0.0001) is a common practice to simulate async behavior in tests.
3. tests/resources/pipe_async.py:22
  • Draft comment:
    The use of await asyncio.sleep(0.0001) is appropriate for simulating async behavior in tests. Ensure this pattern is consistently used for async functions.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The pipe_async.py file defines async functions with the pipe_input and pipe_output decorators, which is appropriate for async operations. The use of await asyncio.sleep(0.0001) is a common practice to simulate async behavior in tests.
4. tests/resources/mutate_async.py:13
  • Draft comment:
    Consider using more descriptive function names for _groupby_a_mutate and _groupby_b_mutate to better convey their purpose.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in tests/resources/mutate_async.py and tests/resources/pipe_async.py is straightforward and does not require additional comments for understanding. However, the function names _groupby_a and _groupby_b could be more descriptive to indicate their purpose better.
5. tests/resources/pipe_async.py:12
  • Draft comment:
    Consider using more descriptive function names for _groupby_a and _groupby_b to better convey their purpose.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in tests/resources/pipe_async.py is straightforward and does not require additional comments for understanding. However, the function names _groupby_a and _groupby_b could be more descriptive to indicate their purpose better.
6. tests/function_modifiers/test_macros.py:1158
  • Draft comment:
    The new test function test_async_pipe_input_and_output_end_to_end is very similar to the existing test_async_mutate_end_to_end. Consider extending the existing test to cover both cases to avoid duplication.

  • function test_async_mutate_end_to_end (test_macros.py)

  • Reason this comment was not posted:
    Marked as duplicate.

7. tests/function_modifiers/test_macros.py:1188
  • Draft comment:
    The new function test_async_mutate_end_to_end is very similar to the existing function test_async_pipe_input_and_output_end_to_end. Consider refactoring to extend the existing function to handle different modules and final variables.

  • function test_async_pipe_input_and_output_end_to_end (test_macros.py)

  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_Lh3AkEPQHfyml3zA


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

tests/function_modifiers/test_macros.py Show resolved Hide resolved
tests/resources/mutate_async.py Show resolved Hide resolved
Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Looks good! We're fixing the tests shortly -- they're failing due to stuff entirely unrelated to your change, we think. Then mind rebasing against main and pushing to the branch (we can double-check that they all pass)?

@jernejfrank
Copy link
Contributor Author

Looks good! We're fixing the tests shortly -- they're failing due to stuff entirely unrelated to your change, we think. Then mind rebasing against main and pushing to the branch (we can double-check that they all pass)?

Sounds good -- FWIW locally the tests were passing.

@skrawcz
Copy link
Collaborator

skrawcz commented Nov 18, 2024

Looks good! We're fixing the tests shortly -- they're failing due to stuff entirely unrelated to your change, we think. Then mind rebasing against main and pushing to the branch (we can double-check that they all pass)?

Sounds good -- FWIW locally the tests were passing.

you should be good to rebase now.

Additional integration tests for async pipe_input, pipe_output and
mutate.
@elijahbenizzy elijahbenizzy merged commit 84493b0 into DAGWorks-Inc:main Nov 18, 2024
24 checks passed
@jernejfrank jernejfrank deleted the async_pipe_tests branch November 28, 2024 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants