[SYNPY-1714] Fix Issues with Failing Tests

[SageGJ]

Problem:

@jaymedina mentioned that Claude had updated code as part of #1251 to fix failing tests but it was not understood why the tests were failing initially or what the change did to resolve the issues that were occurring. Furthermore, the failures were only observed when running in remote environments and not when tested locally.
Notably, this was different from the issues experienced in #1285; the two are not related at all as the former were occurring in the tests covering the client itself while the latter covered the curator extension functionality.

@jaymedina stated that Claude made changes to certain tests, which upon inspection all used a mocked logger and tested the logger with <mocked_logger>.assert_called_with(<warning message>). The change it made was from patching the logger like

with patch("logging.Logger.warning") as mocked_warn:

to patching it like:

with patch.object(self.syn.logger, "warning") as mocked_warn:

Other tests that had patched the logger in the same way were not modified by Claude.

Solution:

It was determined that Claude modified the tests it did because they all used the test assert_called_with. Within the method under test, the warning message was logged through self.logger.warning(<warning message>), with self being an instance of the Synapse object.

assert_called_with checks to ensure that a method is called with the appropriate arguments. Since the logger called was an attribute of an instance, there is an implicity self argument that is passed in before any additional arguments. The change was made to correctly patch the logger object on the synapse instance instead of from the generic Logger class.

This left the question of why the other tests were unmodified and still passing. But since those tests checked for passing with assert_not_called() and neither logging.Logger.warning nor self.syn.logger.warning were called, the test passed because the behavior of the incorrect logger and the correct logger happened to be the same.

These tests were updated to correctly mock the logger as well.

Testing:

The tests now all pass.
Additionally, during development, a warning was added to _check_entity_restrictions to always log a warning. When the object mocking logging.Logger.warning was tested with assert_called_with(), the test failed as expected, indicated that the correct logger was never being tested. When the patch was switched to self.syn.logger and the test rerun, the warning was correctly detected, indicating that the correct object was now being properly mocked and tested.

[SageGJ]

@thomasyu888

[thomasyu888]

🔥 Excellent sleuthing. Turns out we can't trust everything AI after all. Going to leave this to @jaymedina for final review.

[jaymedina]

Thanks so much @SageGJ for your help on this! I just had one comment on something that's unclear to me, otherwise this all looks good!

[thomasyu888]

🔥 Great work. For some reason, the unit tests are failing on GitHub Actions for Python 3.14, and I did a tiny bit of research myself and...

1. The unit tests for curator extensions passed locally (for Python 3.14)
2. The unit tests changed in this file passed locally for me as well (These tests passed locally on the develop branch before & after these changes) for Python 3.14

Just noting this: I give the green light for this to be merged even if not all the tests pass on GitHub.

Separately, when all the features for this upcoming package release are ready, we will do a deep dive into why the tests are unstable and/or failing.

defer to @jaymedina for final review.

[SageGJ]

@thomasyu888 sounds great, and thanks for the investigation.
@jaymedina since this is being merged into your feature branch once you've reviewed it again feel free to merge it yourself when you're ready for it to be added to your other work. Thanks!

[andrewelamb]

🔥 Great work. For some reason, the unit tests are failing on GitHub Actions for Python 3.14, and I did a tiny bit of research myself and...

1. The unit tests for curator extensions passed locally (for Python 3.14)
2. The unit tests changed in this file passed locally for me as well (These tests passed locally on the develop branch before & after these changes) for Python 3.14

Just noting this: I give the green light for this to be merged even if not all the tests pass on GitHub.

Separately, when all the features for this upcoming package release are ready, we will do a deep dive into why the tests are unstable and/or failing.

defer to @jaymedina for final review.

Does this have the fix @jaymedina merged into develop yesterday? If not, that might fix the 3.14-specific failure.

[SageGJ]

@andrewelamb I created this branch from branching off of synpy-1590-submission-model-functionality yesterday afternoon. I'm not sure how recently that branch had been updated from develop, but @jaymedina would know

[jaymedina]

Does this have the fix @jaymedina merged into develop yesterday? If not, that might fix the 3.14-specific failure.

This does have that fix. I rebased/merged my *main and *functionality branches with the latest develop yesterday, and this branch was created off the latest *functionality. I'll try re-running the tests and see if the issue is just flakiness. No more work is needed on your side @SageGJ, thanks for what you've added!

[SageGJ]

Understood, thanks @jaymedina !

[jaymedina]

These failing tests are like whack-a-mole 😂

The 3.14 unit tests still fail, but at least the other unit tests are passing so we can still run the integration tests for previous python versions.

I'll make a ticket for these new failures that I'll move to the top of backlog and try to tackle if I have enough time this sprint after the Submission OOP merge.

Thanks all for authoring/review ✅