#12169 defer: Expand inline callbacks tests #12170

p12tic · 2024-05-06T08:22:28Z

Scope and purpose

This PR significantly expands @inlineCallbacks tests. The code has been taken from #1090.

p12tic · 2024-05-06T08:42:37Z

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.

p12tic · 2024-05-06T10:11:24Z

please review

p12tic · 2024-05-06T10:11:39Z

Failing tests should be rerun - seems like intermittent issue with the infrastructure

src/twisted/internet/test/test_inlinecb.py

src/twisted/newsfragments/12169.bugfix

adiroiban · 2024-05-07T13:12:00Z

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.
If you find a randomly failing tests, please add info about the test name or a link to the build

There are a few open issues for specific flaky tests.

Thanks!

adiroiban

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!

src/twisted/internet/test/test_inlinecb.py

adiroiban

Previous comment was a "request changes" comment.

p12tic · 2024-05-07T13:31:26Z

I am not sure what is the purpose of some of the tests

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.

why for example some lines are not covered

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?

p12tic · 2024-05-07T13:32:07Z

If you find a randomly failing tests, please add info about the test name or a link to the build

It was not the tests themselves that were failing. The infrastructure was bad, e.g. failed to upload things and so on.

adiroiban · 2024-05-07T13:54:42Z

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.

Ok. We can leave moved tests without a docsting.

But, if we don't understand the test, maybe is best to "leave it alone".
That is, don't try to move it.
In this way, it should be easier to follow its blame history.

With the move, I thought that you know what was going on there :)

The infrastructure was bad, e.g. failed to upload things and so on.

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

It seems weird that coverage considers tests as something that needs to be covered.

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

p12tic · 2024-05-07T16:31:48Z

Thanks for explanation, it all makes sense.

codspeed-hq · 2024-05-16T00:23:26Z

CodSpeed Performance Report

Merging #12170 will not alter performance

_{Comparing p12tic:12169-more-inline-callbacks-tests (b5e8b45) with trunk (fd08b87)}

Summary

✅ 2 untouched benchmarks

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.

p12tic · 2024-05-16T00:42:29Z

I've applied all suggestions.

p12tic · 2024-05-16T00:42:33Z

please review

adiroiban

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.

adiroiban · 2024-05-19T09:21:03Z

src/twisted/test/test_defgen.py

-    return runWithWarningsSuppressed(
-        [
-            SUPPRESS(
-                message="twisted.internet.defer.deferredGenerator was " "deprecated"


I see that this was deprecated in 2015 ... I think that is ok to just remove this code rather than keep maintaining it.

src/twisted/test/test_defgen.py

src/twisted/internet/test/test_inlinecb.py

adiroiban · 2024-05-19T09:56:22Z

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.

p12tic force-pushed the 12169-more-inline-callbacks-tests branch 2 times, most recently from 53f6400 to 5ca1828 Compare May 6, 2024 08:26

p12tic force-pushed the 12169-more-inline-callbacks-tests branch 6 times, most recently from fe4d4bb to 9cba611 Compare May 6, 2024 10:00

chevah-robot added the needs-review label May 6, 2024

chevah-robot requested a review from a team May 6, 2024 10:11

p12tic force-pushed the 12169-more-inline-callbacks-tests branch from 9cba611 to 06b9289 Compare May 6, 2024 16:55

adiroiban reviewed May 7, 2024

View reviewed changes

src/twisted/internet/test/test_inlinecb.py Outdated Show resolved Hide resolved

adiroiban reviewed May 7, 2024

View reviewed changes

src/twisted/newsfragments/12169.bugfix Outdated Show resolved Hide resolved

adiroiban reviewed May 7, 2024

View reviewed changes

adiroiban requested changes May 7, 2024

View reviewed changes

chevah-robot added needs-changes and removed needs-review labels May 7, 2024

p12tic force-pushed the 12169-more-inline-callbacks-tests branch 2 times, most recently from f70fbe7 to 8a588fa Compare May 16, 2024 00:20

p12tic added 2 commits May 16, 2024 03:32

defer: Expand inline callbacks tests

093f7a3

p12tic added 4 commits May 16, 2024 03:32

defer: Move test-related helpers to within tests themselves

e56140a

This makes it much clearer which functions belong to which test without additional documentation.

defer: Move duplicate inlineCallbacks tracebacks test to correct module

b7beba9

defer: Move more inlineCallbacks tests to correct module

51f44e4

defer: Add a note of why tests should call certain functions directly

4d22b36

p12tic force-pushed the 12169-more-inline-callbacks-tests branch from 2c77621 to 4d22b36 Compare May 16, 2024 00:32

chevah-robot added needs-review and removed needs-changes labels May 16, 2024

chevah-robot requested a review from a team May 16, 2024 00:42

p12tic changed the title ~~defer: Expand inline callbacks tests~~ #12169 defer: Expand inline callbacks tests May 16, 2024

adiroiban approved these changes May 19, 2024

View reviewed changes

chevah-robot added needs-merge and removed needs-review labels May 19, 2024

Add coverage pragma for lines not expected to be executed.

b5e8b45

adiroiban merged commit c465c46 into twisted:trunk May 19, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#12169 defer: Expand inline callbacks tests #12170

#12169 defer: Expand inline callbacks tests #12170

p12tic commented May 6, 2024

p12tic commented May 6, 2024 •

edited

p12tic commented May 6, 2024

p12tic commented May 6, 2024

adiroiban commented May 7, 2024

adiroiban left a comment

adiroiban left a comment

p12tic commented May 7, 2024

p12tic commented May 7, 2024

adiroiban commented May 7, 2024 •

edited

p12tic commented May 7, 2024

codspeed-hq bot commented May 16, 2024 •

edited

p12tic commented May 16, 2024

p12tic commented May 16, 2024

adiroiban left a comment

adiroiban May 19, 2024

adiroiban commented May 19, 2024

#12169 defer: Expand inline callbacks tests #12170

#12169 defer: Expand inline callbacks tests #12170

Conversation

p12tic commented May 6, 2024

Scope and purpose

p12tic commented May 6, 2024 • edited

p12tic commented May 6, 2024

p12tic commented May 6, 2024

adiroiban commented May 7, 2024

adiroiban left a comment

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

p12tic commented May 7, 2024

p12tic commented May 7, 2024

adiroiban commented May 7, 2024 • edited

p12tic commented May 7, 2024

codspeed-hq bot commented May 16, 2024 • edited

CodSpeed Performance Report

Merging #12170 will not alter performance

Summary

p12tic commented May 16, 2024

p12tic commented May 16, 2024

adiroiban left a comment

Choose a reason for hiding this comment

adiroiban May 19, 2024

Choose a reason for hiding this comment

adiroiban commented May 19, 2024

p12tic commented May 6, 2024 •

edited

adiroiban commented May 7, 2024 •

edited

codspeed-hq bot commented May 16, 2024 •

edited