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

Cleanup some useless mocks/fixture in manager tests #45340

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

Conversation

jedcunningham
Copy link
Member

This fixture was, in theory, able to reproduce the bug fixed in #30243, but I was unable to now. So, let's toss it.

cc @uranusjr @blinkseb, not sure if either of you might know why?

@uranusjr
Copy link
Member

uranusjr commented Jan 2, 2025

I don’t remember the details, but from the things mocked, it looks like we changed when we filter files/folders from a scan to check whether a file actually contains a DAG later in the manager, making the mocks now useless (because they are no longer called at this part of the code). All of the mocks simply forces the manager to treat a Python file as one containing a DAG and not drop it prematurely.

@potiuk
Copy link
Member

potiuk commented Jan 2, 2025

@jedcunningham - can you please rebase that one -> we found and issue with @jscheffl with the new caching scheme - fixed in #45347 that would run "main" version of the tests. I am asking in all affected PRs to rebase.

This fixture was, in theory, able to reproduce the bug fixed in apache#30243,
but I was unable to now. So, let's toss it.
@jedcunningham jedcunningham force-pushed the cleanup_parsing_manager_tests branch from 50131bd to a94e79e Compare January 2, 2025 14:28
@jedcunningham jedcunningham added full tests needed We need to run full set of tests for this PR to merge and removed full tests needed We need to run full set of tests for this PR to merge labels Jan 2, 2025
@jedcunningham
Copy link
Member Author

@uranusjr, sorry, to clarify I meant the change_platform_timezone fixture - removing the code fix from #30243 didn't cause the tests to fail for me.

@jedcunningham
Copy link
Member Author

Messed with this a bit more, still not able to get the tests to fail. I say we go ahead with removing it.

@jedcunningham jedcunningham marked this pull request as ready for review January 2, 2025 17:20
@blinkseb
Copy link
Contributor

blinkseb commented Jan 2, 2025

Hi! I don't remember the full details, but as mentioned in #30243, it's normal that the tests don't fail when removing the fixture since the bug was fixed. The fixture was there to ensure there would be no regression (past or future) by assuming that the system timezone is not the same as the configured user timezone.

@jedcunningham
Copy link
Member Author

I left the fixture, I undid the bugfix. I also verified the timestamp didn't match the UTC timestamp, but still the tests are passing. Something else must have changed since that PR landed, but unfortunately that fixture doesn't seem to be preventing a regression any longer.

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.

5 participants