-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
#12169 defer: Expand inline callbacks tests #12170
#12169 defer: Expand inline callbacks tests #12170
Conversation
53f6400
to
5ca1828
Compare
I have also added an additional commit 1debeec and some further commits that moves some inline callbacks tests from other files to test_inlinecb.py. It can be regarded as adding more tests too as it certainly makes them more discoverable. |
fe4d4bb
to
9cba611
Compare
please review |
Failing tests should be rerun - seems like intermittent issue with the infrastructure |
9cba611
to
06b9289
Compare
Which tests were failing and required a re-run ? I can see that all the tests have passed from the first run - https://github.com/twisted/twisted/actions/runs/8972762885/job/24641482445?pr=12170 We are trying to reduce the numer of flaky/random failing test. There are a few open issues for specific flaky tests. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for the changes.
Happy to see more tests and cleanups.
Can you please add more docstring to the tests?
I am not sure what is the purpose of some of the tests and why for example some lines are not covered.
If a line is expected not to be covered, it should be marked with # pragma: no cover
Also, I am very happy to have the test move outside of the #1090 PR
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous comment was a "request changes" comment.
I will add docstrings where I can. Some of the tests were just moved, so I don't understand them either, but I will try.
It seems weird that coverage considers tests as something that needs to be covered. Perhaps the configuration is broken. As I understand, tests are what creates coverage for other tests, they themselves shouldn't be covered. What I'm missing here? |
It was not the tests themselves that were failing. The infrastructure was bad, e.g. failed to upload things and so on. |
Ok. We can leave moved tests without a docsting. But, if we don't understand the test, maybe is best to "leave it alone". With the move, I thought that you know what was going on there :)
No problem. If it happens to see such an error again, Is best to leave a link so that we can take a look and see what can be done to improve the infrastructrure
I consider that coverage for the test code should be always 100% mandatory... no exception. It helps detect tests or part of the tests that are never called. For example we have tests skipped under various conditions, but we should make sure that a test is always executed at least once...that is, it is newer always skipped. I was not asking to have tests for the testing function / helpers. I was asking to make sure that any testing function or helper is alwasys executed at least one. Does it make sense ? Regards |
Thanks for explanation, it all makes sense. |
f70fbe7
to
8a588fa
Compare
CodSpeed Performance ReportMerging #12170 will not alter performanceComparing Summary
|
deferredGenerator and inlineCallbacks tests just happen to be similar enough that some of them can be shared. However, this sharing is ephemeral because there's no underlying reason why both sets of tests should change both at the same time. Changes to how inlineCallbacks works will affect only that part of the test suite and changes to how deferredGenerator will affect only its part of the test suite as well. Duplicating the tests makes them easier to discover and understand. Additionally, this will need to be done at some point anyway because deferredGenerator has been deprecated.
This makes it much clearer which functions belong to which test without additional documentation.
2c77621
to
4d22b36
Compare
I've applied all suggestions. |
please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for moving the inline tests, outside of the deferedGenerator.
I think that we can delete the deprecated code for waitForDeferred and deferedGenerator.
The only comment is about informing coverage about lines that are exected not to be executed.
return runWithWarningsSuppressed( | ||
[ | ||
SUPPRESS( | ||
message="twisted.internet.defer.deferredGenerator was " "deprecated" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this was deprecated in 2015 ... I think that is ok to just remove this code rather than keep maintaining it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
Thanks for the cleanup. I have merged this as I hope it will help with future cleanup work for deprecated code and with deferred performance improvements. |
Scope and purpose
Fixes #12169
This PR significantly expands
@inlineCallbacks
tests. The code has been taken from #1090.