-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
base: main
Are you sure you want to change the base?
Cleanup some useless mocks/fixture in manager tests #45340
Conversation
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. |
@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.
50131bd
to
a94e79e
Compare
Messed with this a bit more, still not able to get the tests to fail. I say we go ahead with removing it. |
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. |
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. |
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?