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

test: Add test for lazy connection filtering issue #539

Closed
wants to merge 4 commits into from

Conversation

Eraldo
Copy link
Contributor

@Eraldo Eraldo commented Jun 1, 2024

#535

Description

Added reproduction test for #535
I marked the test ask skipped until a fix is in place. ;)

Types of Changes

  • Test for issue

Issues Fixed or Closed by This PR

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Eraldo - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 2 issues found
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -1175,3 +1177,22 @@ def test_query_connection_total_count_sql_queries(
assert result.data == {
query_attr: {"totalCount": 5},
}


@pytest.mark.skip(reason="TODO: Fix issue => https://github.com/strawberry-graphql/strawberry-django/issues/535")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Consider adding a test for the issue once it is fixed

While the test is currently skipped due to the issue, it would be beneficial to add a test case that verifies the fix once the issue is resolved. This will ensure that the problem does not reoccur in the future.

Suggested change
@pytest.mark.skip(reason="TODO: Fix issue => https://github.com/strawberry-graphql/strawberry-django/issues/535")
@pytest.mark.skip(reason="TODO: Fix issue => https://github.com/strawberry-graphql/strawberry-django/issues/535")
def test_query_lazy_connection_with_filters(db):
color = Color.objects.create(name="Magenta")
@pytest.mark.skipif(not issue_535_fixed(), reason="Issue 535 not fixed yet")
def test_query_lazy_connection_with_filters_fixed(db):
color = Color.objects.create(name="Magenta")

}
}
"""
result = schema.execute_sync(query=query)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Check for expected data in the result

Currently, the test only checks for the absence of errors. It would be more robust to also verify that the returned data matches the expected structure and values. This ensures that the query not only executes without errors but also returns the correct data.

Suggested change
result = schema.execute_sync(query=query)
result = schema.execute_sync(query=query)
assert result.errors is None
expected_data = {
# Add the expected structure and values here
}
assert result.data == expected_data

Fruit.objects.create(name="Apple", color=color)
query = """
query TestQuery {
colorWithLazyFruits(filters: {}) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm checking here and this is not supposed to have filters. What is supposed to have filters is fruits, and in my tests it seems to have

@bellini666
Copy link
Member

Going to close this as it is not actually reproducing the issue.

@bellini666 bellini666 closed this Jun 6, 2024
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.

2 participants