Add clarification and test to fixed SSS behaviour#19734
Conversation
erikjohnston
force-pushed
the
erikj/sss_clarification
branch
2 times, most recently
from
1d9fdc1 to
3149105
Compare
erikjohnston
force-pushed
the
erikj/sss_clarification
branch
from
3149105 to
f86094c
Compare
| # Advance past the timeout to make sure the request finishes. (We do this | ||
| # to ensure log contexts don't leak between tests). |
There was a problem hiding this comment.
Which logcontext is being leaked here?
Here was my gut reaction:
Overall, we shouldn't be worrying about LoggingContext problems in the tests,
[...] it's expected that test things run outside of Synapse and by their nature should happen in the
sentinellogcontext. And if we really don't want to see these kind of errors, we would need to refactorget_success(...)to look more likeget_success(func, OTHER_SERVER, ev)where we call the function with somehomeserver_test_caselogcontext instead of passing in an awaitable.
There was a problem hiding this comment.
This was causing test failures. I'm not entirely sure what was going on, but the trial jobs started failing when running in parallel
There was a problem hiding this comment.
Finishing a request to be a good citizen seems fine. The extra complexity in the test is a bit distracting and slightly tricky to figure out that it isn't related to what we're testing. Perhaps a better comment could dispel things better as I read but the current language is fine ⏩
Also makes me wonder what happens if the test fails in the middle of a request (seems like we would run into the same problem).
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
| # Advance past the timeout to make sure the request finishes. (We do this | ||
| # to ensure log contexts don't leak between tests). |
There was a problem hiding this comment.
Finishing a request to be a good citizen seems fine. The extra complexity in the test is a bit distracting and slightly tricky to figure out that it isn't related to what we're testing. Perhaps a better comment could dispel things better as I read but the current language is fine ⏩
Also makes me wonder what happens if the test fails in the middle of a request (seems like we would run into the same problem).
…j/sss_clarification
2 participants
Follow on from #19714 (-> #19792), where we should have had an extra comment and test.
cc @MadLittleMods
Dev notes
required_statearen't sent until other activity occurs #18844 (comment)