[#10331] Re-enable Azure DevOps Windows and macOS tests. #1719

adiroiban · 2022-04-11T18:31:45Z

Scope and purpose

See if we can resurect Azure CI.

Remove py3.6 as it looks like it's no longer supported by Azure

Try to fix or skip the failing tests.

I tried to skip only on CI, so if a Twisted dev runs the test suite on mac or Windows, those tests will not be automatically skipped.

For the skipped tests, I have only left a comment without create separate tickets.
I don't have any hope that in the future someone will search the tickets hunting for tests that can be re-enabled.
Rather, someone found a bug, will see the existing test and then try to see if the test can be fixed as part of the current bugfix effort.

Reduce the size of Windows and MacOS CI matrix to reduce flaky reports.
At this point, we don't have active Twisted developers on mac and Windows.

Contributor Checklist:

The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10331
I ran tox -e lint to format my patch to meet the Twisted Coding Standard
I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
The title of the PR starts with the associated Trac ticket number (without the # character).
[NA] I have updated the automated tests and checked that all checks for the PR are green.
I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
The merge commit will use the below format
The first line is automatically generated by GitHub based on PR ID and branch name.
The other lines generated by GitHub should be replaced.

Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: adiroiban
Reviewer: 
Fixes: ticket:10331

Re-enable Windows and MacOS CI tests.

exarkun

Assuming the Azure jobs pass, this looks good to me.

adiroiban · 2022-04-12T12:16:40Z

src/twisted/internet/test/test_posixbase.py

    def test_noWakerConstructionWarnings(self):
+        """
+        No warnings are generated when constructing the waker.


this is a test failing on Windows.
it was failing as it checked there are no warnings.

But there were warnings that were accumulated by running other tests.

I re-wrote a bit the test while trying to understand what it does.

adiroiban · 2022-04-12T12:17:52Z

src/twisted/internet/test/test_posixbase.py

@@ -22,6 +23,29 @@
 from twisted.internet.tcp import Port


+class WarningCheckerTestCase(TestCase):


This was added trying to catch tests producing warnings, but not consuming the warnings.

I left this code here as maybe it can help with future debugging... and help localize tests with side-effects.

exarkun

I don't think I understand the skipping strategy. I left some words inline about this but there are other instances I skipped over because I suppose the answer will be the same.

src/twisted/conch/test/test_filetransfer.py

src/twisted/internet/test/test_posixbase.py

exarkun · 2022-04-12T13:23:45Z

src/twisted/internet/test/test_posixbase.py

+        self.assertEqual(len(warnings), 0, warnings)
+
+        # Explicitly close the sockets as a cleanup and make sure we
+        # don't get other warnings here.


I'm not sure what "as a cleanup" means here. The word "cleanup" suggests "TestCase.addCleanup" but then the code just goes ahead and closes the socket immediately. So maybe just remove "as a cleanup"?

Or maybe it really should use TestCase.addCleanup but that should happen on line 333 so that it happens even if the assertion on line 335 fails.

I tried to rephrase. thanks.

exarkun · 2022-04-12T13:25:00Z

src/twisted/internet/test/test_process.py

@@ -71,6 +71,28 @@ def onlyOnPOSIX(testMethod):
    return testMethod


+def onlyOnLinuxCI(testMethod):
+    """
+    Only run this test on Linux and macOS local tests and Linux CI platforms.


Again I don't understand why there is a CI check here. Why isn't the logic to run on Linux and skip on macOS and Windows, regardless of whether it is a CI environment or not?

exarkun · 2022-04-12T13:26:00Z

src/twisted/internet/test/test_process.py

+        testMethod.skip = "Failing on Azure macOS CI."
+
+    if resource is None:
+        testMethod.skip = "Test only applies to POSIX platforms."


For actually doing the skips, it seems like skipIf(not platform.isLinux(), "reason") is the conventional idiom.

True. I have refactored it...and the CI check was an error.

adiroiban

Thanks for the review.
I tried to update the PR based on your feedback.

I haven't refactored WarningCheckerTestCase into a fixture due to the fact that it would mean re-implementing flushWarnings

I hope this is ready to land now.

adiroiban · 2022-04-12T13:56:31Z

src/twisted/internet/test/test_posixbase.py

+        self.assertEqual(len(warnings), 0, warnings)
+
+        # Explicitly close the sockets as a cleanup and make sure we
+        # don't get other warnings here.


I tried to rephrase. thanks.

adiroiban · 2022-04-12T14:14:22Z

src/twisted/internet/test/test_process.py

+        testMethod.skip = "Failing on Azure macOS CI."
+
+    if resource is None:
+        testMethod.skip = "Test only applies to POSIX platforms."


True. I have refactored it...and the CI check was an error.

src/twisted/internet/test/test_posixbase.py

exarkun

Thanks. Looks good to me. A few trivial comments inline to fix, but then please merge.

exarkun · 2022-04-14T19:05:17Z

src/twisted/internet/test/test_posixbase.py

+        # https://twistedmatrix.com/trac/ticket/10332
+        # For now, try to start each test without previous warnings
+        # on Windows CI environment.
+        # We still want to see failures on local Windows development environment to make it easier to fix then,


Typo at the end - "then"

exarkun · 2022-04-14T19:05:28Z

src/twisted/internet/test/test_posixbase.py

+                # https://twistedmatrix.com/trac/ticket/10332
+                # For now don't raise errors on Windows as the existing tests are dirty and we don't have the dev resources to fix this.
+                # If you care about Twisted on Windows, enable this check and hunt for the test that is generating the warnings.
+                # Note that even with this check disabled, you can still see flaky tests on Windows, as due due stray delayed calls


Typo - "as due due stray"

exarkun · 2022-04-14T19:06:45Z

src/twisted/internet/test/test_process.py

+    # We still want to run it on local development macOS environments to help developers discover and fix this issue.
+    @skipIf(
+        platform.isMacOSX() and os.environ.get("CI", "").lower() == "true",
+        "Skipped on macOS CI en.",


not sure about "en" - typo? maybe "env"?

…nto 10331-win-mac-ci-azure

Remove py3.6.

84c6174

exarkun approved these changes Apr 11, 2022

View reviewed changes

adiroiban added 3 commits April 11, 2022 22:39

Add some docs about Azure DevOps files. Reduce matrix size.

a671985

See if we can fix tests on win and mac.

a243145

Try more to fix Azure tests.

55013b2

adiroiban force-pushed the 10331-win-mac-ci-azure branch from b4d3e95 to 55013b2 Compare April 12, 2022 11:23

adiroiban added 2 commits April 12, 2022 12:42

See which test is leaving the test environment dirty.

1f73e8c

Try to disable side-effect checks on Windows.

8ec2327

adiroiban commented Apr 12, 2022

View reviewed changes

adiroiban added 2 commits April 12, 2022 13:22

Try more to clean side-effect from other tests.

8691be9

Skip only on CI.

a2c81ea

adiroiban requested review from exarkun and a team April 12, 2022 13:17

exarkun reviewed Apr 12, 2022

View reviewed changes

Update after review.

68f9e90

adiroiban commented Apr 12, 2022

View reviewed changes

Merge branch 'trunk' into 10331-win-mac-ci-azure

51bc4e1

exarkun approved these changes Apr 14, 2022

View reviewed changes

adiroiban added 2 commits April 14, 2022 20:10

Fix typos.

95e1302

Merge branch '10331-win-mac-ci-azure' of github.com:twisted/twisted i…

e494ec7

…nto 10331-win-mac-ci-azure

adiroiban enabled auto-merge April 14, 2022 19:11

adiroiban merged commit 2077cca into trunk Apr 14, 2022

adiroiban deleted the 10331-win-mac-ci-azure branch July 19, 2022 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#10331] Re-enable Azure DevOps Windows and macOS tests. #1719

[#10331] Re-enable Azure DevOps Windows and macOS tests. #1719

adiroiban commented Apr 11, 2022 •

edited

exarkun left a comment

adiroiban Apr 12, 2022

adiroiban Apr 12, 2022

exarkun left a comment

exarkun Apr 12, 2022

adiroiban Apr 12, 2022

exarkun Apr 12, 2022

exarkun Apr 12, 2022

adiroiban Apr 12, 2022

adiroiban left a comment

adiroiban Apr 12, 2022

adiroiban Apr 12, 2022

exarkun left a comment

exarkun Apr 14, 2022

exarkun Apr 14, 2022

exarkun Apr 14, 2022

		@@ -22,6 +23,29 @@
		from twisted.internet.tcp import Port


		class WarningCheckerTestCase(TestCase):

[#10331] Re-enable Azure DevOps Windows and macOS tests. #1719

[#10331] Re-enable Azure DevOps Windows and macOS tests. #1719

Conversation

adiroiban commented Apr 11, 2022 • edited

Scope and purpose

Contributor Checklist:

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented Apr 11, 2022 •

edited