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

fix test_filters to use set to compare dag id's in dag run tests #44391

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rawwar
Copy link
Collaborator

@rawwar rawwar commented Nov 26, 2024

CI failed for test_filters in test_dag_run.py(fastapi).

This PR fixes it

Issue. Test should get all dag runs with "success" state. But, the order of these dag run's isn't relevant to this filter test. Hence, using a set instead of list.

EDIT: Looks like this is an issue across tests for list dag_run. I'll update this PR to fix all of its tests.

@rawwar rawwar marked this pull request as ready for review November 26, 2024 15:38
@rawwar rawwar requested a review from potiuk November 26, 2024 15:38
@rawwar rawwar marked this pull request as draft November 26, 2024 15:46
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

I do not agree with that. Ordering of items returned by the API should be deterministic. Fixing the flakiness like this does not actually solve the problem I belive.

@potiuk
Copy link
Member

potiuk commented Nov 26, 2024

I do not agree with that. Ordering of items returned by the API should be deterministic. Fixing the flakiness like this does not actually solve the problem I belive.

I agree it's a better solution

@uranusjr
Copy link
Member

I don’t know about the new implementation, but in the Connextion implementation, the result is actually sorted, but it’s against a very weird default (id) that isn’t really deterministic in the tests because when you insert things through the ORM it’s not guaranteed the id would be the same as the order you insert them (due to both database and ORM implementation). So the tests will need fixing in any case unless we change the default.

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.

4 participants