Skip to content

Conversation

@nailo2c
Copy link
Contributor

@nailo2c nailo2c commented Jun 14, 2025

Yesterday, a PR I submitted adding a unit test for the serialize-related function was merged. However, I'm not sure why this unit test fails in CI when running the following command: breeze testing core-tests --use-xdist --skip-db-tests --no-db-cleanup --backend none

For now, I'm removing this test, as I've noticed it is causing failures in several PRs.

Affected PRs:
#51701
#51735
#51698

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

@jscheffl jscheffl merged commit 463a716 into apache:main Jun 14, 2025
52 checks passed
@potiuk
Copy link
Member

potiuk commented Jun 14, 2025

I am not sure if removing failed tests because it fails is a good idea.

@jscheffl
Copy link
Contributor

I am not sure if removing failed tests because it fails is a good idea.

I as well... usually... but I am wonderuing how this test passed initially in https://github.com/apache/airflow/actions/runs/15639878625 and from then on failed on main.
So if it is a new and unstable test we should revert and properly re-implment.

We should not remove tests that are long standing.

@potiuk
Copy link
Member

potiuk commented Jun 14, 2025

Likely because it was behind:

Screenshot 2025-06-14 at 22 49 30

But I think we should in this case let the author - @nailo2c - can you take a look and check and retry it ?

@potiuk
Copy link
Member

potiuk commented Jun 14, 2025

(BTW. @nailo2c -> noting bad -> it happens to every one of us :). I just don't like to reverse changes without a firm "I know why" and "there is a plan to fix it" :)

@nailo2c
Copy link
Contributor Author

nailo2c commented Jun 14, 2025

Got it, let me take a look on it, and thanks for the help! :)

@nailo2c
Copy link
Contributor Author

nailo2c commented Jun 14, 2025

It was likely caused by behind, I checked out a new branch from the latest main and the test passed again.

The logic of this unit test is quite simple, let me dig in and try to find out why his happened, then maybe submit another PR to add this test back?

Unit test:

def test_serde_serialize_recursion_limit(self):
    depth = sys.getrecursionlimit() - 1
    with pytest.raises(RecursionError, match="maximum recursion depth reached for serialization"):
        serialize(object(), depth=depth)

Tested function:

MAX_RECURSION_DEPTH = sys.getrecursionlimit() - 1
def serialize(o: object, depth: int = 0) -> U | None:
    if depth == MAX_RECURSION_DEPTH:
        raise RecursionError("maximum recursion depth reached for serialization")

@kaxil
Copy link
Member

kaxil commented Jun 16, 2025

It was likely caused by behind, I checked out a new branch from the latest main and the test passed again.

The logic of this unit test is quite simple, let me dig in and try to find out why his happened, then maybe submit another PR to add this test back?

Unit test:

def test_serde_serialize_recursion_limit(self):
    depth = sys.getrecursionlimit() - 1
    with pytest.raises(RecursionError, match="maximum recursion depth reached for serialization"):
        serialize(object(), depth=depth)

Tested function:

MAX_RECURSION_DEPTH = sys.getrecursionlimit() - 1
def serialize(o: object, depth: int = 0) -> U | None:
    if depth == MAX_RECURSION_DEPTH:
        raise RecursionError("maximum recursion depth reached for serialization")

Could you create a GH issue in the meantime, so it isn't forgotten?

@nailo2c
Copy link
Contributor Author

nailo2c commented Jun 16, 2025

sure, let me create a GH issue later

@nailo2c
Copy link
Contributor Author

nailo2c commented Jun 19, 2025

The GH issue: #51915

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants