CI: Enable testing on macOS and Windows by amotl · Pull Request #707 · caronc/apprise

amotl · 2022-10-17T11:42:44Z

Hi again,

this patch addresses some shortcomings when it comes to making the test suite succeed on Windows. Mostly, it is about replacing the NotifySyslog notifier with another one within some test cases, because the syslog package is not present on Windows installations of Python.

Other than this, it uses the certifi package to add an operating system agnostic source to the list of root certificate authority (CA) bundle candidates, CA_CERTIFICATE_FILE_LOCATIONS, for NotifyMQTT.

Visually, it will look like outlined below.

With kind regards,
Andreas.

[1]

Appendix: CI/GHA jobs view

-- https://github.com/caronc/apprise/actions/runs/3277660846/usage

Appendix: First successful run

-- https://github.com/panodata/apprise/actions/runs/3264695345/usage

For daily business, it will be limited to a single slot for macOS and Windows. See discussion below.

amotl · 2022-10-17T11:43:33Z

Unfortunately, running the tests with coverage on PyPy fails on Windows, so we had to exclude the corresponding jobs from the test matrix with a85eabc.

D:\a\_temp\97f83735-bb42-4fd2-8f39-f55f4c9e8dcd.sh: line 1:  1240 Segmentation fault      coverage run -m pytest

-- https://github.com/panodata/apprise/actions/runs/3261675617/jobs/5357105180

codecov-commenter · 2022-10-18T11:40:11Z

Codecov Report

Base: 99.98% // Head: 99.98% // No change to project coverage 👍

Coverage data is based on head (1c6eb49) compared to base (4fc4b8e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #707   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files         113      113           
  Lines       14533    14539    +6     
  Branches     2960     2961    +1     
=======================================
+ Hits        14531    14537    +6     
  Misses          1        1           
  Partials        1        1

Impacted Files	Coverage Δ
apprise/plugins/NotifyMQTT.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

caronc · 2022-10-18T21:20:10Z

For Windows testing, I'm not sure if we have to go beyond just picking a single flavor of Python and rolling with that. It's absolutely amazing that you got it to work with all version though.

What are your thoughts here?

amotl · 2022-10-18T22:44:25Z

Single-platform builds for daily business

For Windows testing, I'm not sure if we have to go beyond just picking a single flavor of Python and rolling with that. What are your thoughts here?

I fully agree with this, see #702 (comment):

I believe it will be crazy to run macOS- and Windows-tests on the whole test matrix. I think it will be good enough to restrain them to Python 3.10 only, for example.

Full matrix once

It's absolutely amazing that you got it to work with all version though.

Thanks. I just wanted to see it working at least once, before I will limit it again.

amotl · 2022-10-18T23:25:28Z

Picking a single flavor of Python [...]

5056ea0 brings all into an appropriate shape now.

I decided to group the jobs into two halves: The first one, let's call it "primary", runs CPython 3.10 on all variants of (standard, bare, macOS, and Windows). The second one will spawn the full test matrix on the CPython version axis on Linux.

caronc · 2022-10-19T00:04:31Z

apprise/plugins/NotifyMQTT.py

        # return results
        return results
+
+    @property


This is great, but if you grep for pki (for example), this list is used in several locations. The Todo in place was more to move this into the utils.py' or maybe a new file called secure.py`. A general reuse locating basically.

It would allow reuse. I need to verify how many other spots there are though. I think it was used for xmpp too, but that was dropped for now. I still like the way you massaged it.

If you grep for pki (for example), this list is used in several locations.

I just tried, but did not find any other occurrances.

I think it was used for xmpp too, but that was dropped for now.

Ah. That makes sense!

The todo in place was more to move this into the utils.py or maybe a new file called secure.py.

I don't think it will need to go to a dedicated file. Let's refactor it when needed.

I still like the way you massaged it.

Thanks. It only needed to be done in order to be able to add certifi.where(). It would not have been possible within the body of the class.

amotl · 2022-10-18T23:57:41Z

test/test_locale.py

+@pytest.mark.skipif(sys.platform == "win32", reason="Does not work on Windows")
+def test_detect_language_windows_users_croaks_please_review():
+    """
+    When enabling CI testing on Windows, those tests did not produce the
+    correct results. They may want to be reviewed.
+    """


I moved some tests from test_locale.py into this test case function, because they did not produce the expected outcomes on Windows. You may want to review those now, or postpone it into the future. d5f07a3 is the corresponding commit which was needed here.

amotl · 2022-10-19T00:00:20Z

test/test_logger.py

-        # Remove our file before we exit the with clause
-        # this causes our delete() call to throw gracefully inside
-        os.unlink(str(log_file))
+        # Concurrent file access is not possible on Windows.
+        # PermissionError: [WinError 32] The process cannot access the file
+        # because it is being used by another process.
+        if sys.platform != "win32":
+            # Remove our file before we exit the with clause
+            # this causes our delete() call to throw gracefully inside
+            os.unlink(str(log_file))

-        # Verify file is gone
-        assert not os.path.isfile(str(log_file))
+            # Verify file is gone
+            assert not os.path.isfile(str(log_file))


I don't know what was intended to be tested here, but like it's done here will croak on Windows. The reason is because concurrent file access is not possible on Windows.

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process.

43dd1be fixes it.

amotl · 2022-10-19T00:23:08Z

There has also been a fluke on the most recent test run, see #692 (comment). Unfortunately, I am not able to give it a kick, so I will force-push to re-run completely.

caronc · 2022-10-20T00:15:32Z

This is amazing work @amotl ! I would only ask that we drop the certifi. I don't quite see the value in it since the pem is also updated through the os distributions. If this is only for windows, maybe stick it in the win-requirements.txt file?

It's a small request as it just keeps dependencies really low. Otherwise 😲... just awsome work; it's an easy merge thereafter.

Thoughts?

amotl · 2022-10-30T19:21:44Z

Dear Chris,

I would only ask that we drop the certifi. I don't quite see the value in it since the pem is also updated through the os distributions.

The value is that it will work universally on all platforms where Apprise can be installed, not only on the Linux-based systems explicitly enumerated there. Other than this, the certifi package is already a dependency of the requests package ¹, adding it to Apprise's list of dependencies just makes it explicit.

I think the certifi package has exactly been conceived for that very purpose of providing a root CA certificate bundle in an OS-agnostic manner, so I would even recommend to exclusively use it for this purpose within Apprise, and drop the other variants.

I also would like to recommend to add an option to let the user provide a path to a certificate bundle, in the specific context of MQTT. In contrast to HTTP, there isn't really a public infrastructure of MQTT brokers out there. When using MQTT with TLS encryption, users will mostly run their own PKIs.

With kind regards,
Andreas.

https://github.com/psf/requests/blob/v2.28.1/setup.py#L65 ↩

`detect_language` yields `en` here.

Concurrent file access is not possible on Windows: PermissionError: [WinError 32] The process cannot access the file because it is being used by another process.

…review` When enabling CI testing on Windows, some tests did not produce the correct results. They may want to be reviewed. All those test cases are now bundled within this function, in order to make CI succeed.

This will support all Python installations on platforms where the operating system does not provide such a certificate bundle.

This will save resources when pushing to the same PR branch multiple times in (quick) succession. Mostly, there is no need to finish the workflow jobs which are currently running.

ERROR: Could not find a version that satisfies the requirement pywin32 (from versions: none) ERROR: No matching distribution found for pywin32

When invoking the tests using `coverage` on Windows/PyPy, it segfaults.

amotl mentioned this pull request Oct 17, 2022

CI: Add GHA setup #702

Merged

amotl force-pushed the amo/windows branch 2 times, most recently from 97a35b4 to f817d1e Compare October 18, 2022 11:39

amotl changed the title ~~[STACK] Make testing work on Windows~~ [WIP] Make testing work on Windows Oct 18, 2022

amotl mentioned this pull request Oct 18, 2022

Tests: Collection of flukes on CI #692

Closed

amotl force-pushed the amo/windows branch 4 times, most recently from 11f631f to e6a006d Compare October 18, 2022 13:57

amotl force-pushed the amo/windows branch 2 times, most recently from 092364a to b9b03a7 Compare October 18, 2022 23:07

amotl changed the title ~~[WIP] Make testing work on Windows~~ CI: Enable testing on macOS and Windows Oct 18, 2022

amotl force-pushed the amo/windows branch from a6a3e6e to e497ab8 Compare October 18, 2022 23:19

amotl force-pushed the amo/windows branch 2 times, most recently from 5056ea0 to 2e72374 Compare October 18, 2022 23:51

caronc reviewed Oct 19, 2022

View reviewed changes

amotl force-pushed the amo/windows branch from 2e72374 to 274bc57 Compare October 19, 2022 00:04

amotl commented Oct 19, 2022

View reviewed changes

amotl force-pushed the amo/windows branch from 274bc57 to 4984efa Compare October 19, 2022 00:14

amotl force-pushed the amo/windows branch from 4984efa to dc43ee0 Compare October 19, 2022 00:23

amotl marked this pull request as ready for review October 19, 2022 00:31

amotl requested a review from caronc October 19, 2022 00:31

amotl added 14 commits October 30, 2022 12:32

Tests/Windows: Fix test_attach_file

70cdd28

Tests/Windows: Fix windll referenced before assignment

c902580

Tests/Windows: Disable test_detect_language_defaultlocale

aea2f53

`detect_language` yields `en` here.

Tests/Windows: Fix test_apprise_log_file_captures

a726c81

Concurrent file access is not possible on Windows: PermissionError: [WinError 32] The process cannot access the file because it is being used by another process.

Tests/Windows: Fix test for NotifyWindows

18bbcd2

Tests/Windows: Add `test_detect_language_windows_users_croaks_please_…

a924fdd

…review` When enabling CI testing on Windows, some tests did not produce the correct results. They may want to be reviewed. All those test cases are now bundled within this function, in order to make CI succeed.

NotifyMQTT: Add certifi.where() to list of root CA bundle candidates

337a6e6

This will support all Python installations on platforms where the operating system does not provide such a certificate bundle.

CI/CodeQL: Cancel in-progress jobs when pushing to the same branch

8dce5bb

This will save resources when pushing to the same PR branch multiple times in (quick) succession. Mostly, there is no need to finish the workflow jobs which are currently running.

CI: Always use Bash, also on Windows

d9f83bd

CI: Do not install pywin32 on PyPy

5dbc408

ERROR: Could not find a version that satisfies the requirement pywin32 (from versions: none) ERROR: No matching distribution found for pywin32

CI: Install dbus-python only on Linux

50ae86c

CI: Do not run Windows tests on PyPy

3616d04

When invoking the tests using `coverage` on Windows/PyPy, it segfaults.

CI: Define full test matrix, using a single slot for macOS and Windows

281bdcf

CI: Python 3.11 GA has been released

1c6eb49

amotl force-pushed the amo/windows branch from dc43ee0 to 1c6eb49 Compare October 30, 2022 19:34

caronc merged commit cddd5c4 into caronc:master Oct 30, 2022

caronc deleted the amo/windows branch October 30, 2022 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CI: Enable testing on macOS and Windows#707

CI: Enable testing on macOS and Windows#707
caronc merged 18 commits intocaronc:masterfrom
panodata:amo/windows

amotl commented Oct 17, 2022 •

edited

Loading

Uh oh!

amotl commented Oct 17, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 18, 2022 •

edited

Loading

Uh oh!

caronc commented Oct 18, 2022

Uh oh!

amotl commented Oct 18, 2022

Uh oh!

amotl commented Oct 18, 2022 •

edited

Loading

Uh oh!

caronc Oct 19, 2022

Uh oh!

amotl Oct 19, 2022 •

edited

Loading

Uh oh!

amotl Oct 18, 2022 •

edited

Loading

Uh oh!

amotl Oct 19, 2022

Uh oh!

amotl commented Oct 19, 2022

Uh oh!

caronc commented Oct 20, 2022

Uh oh!

amotl commented Oct 30, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

amotl commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amotl commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

caronc commented Oct 18, 2022

Uh oh!

amotl commented Oct 18, 2022

Single-platform builds for daily business

Full matrix once

Uh oh!

amotl commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caronc Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

amotl Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amotl Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amotl Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

amotl commented Oct 19, 2022

Uh oh!

caronc commented Oct 20, 2022

Uh oh!

amotl commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amotl commented Oct 17, 2022 •

edited

Loading

amotl commented Oct 17, 2022 •

edited

Loading

codecov-commenter commented Oct 18, 2022 •

edited

Loading

amotl commented Oct 18, 2022 •

edited

Loading

amotl Oct 19, 2022 •

edited

Loading

amotl Oct 18, 2022 •

edited

Loading

amotl commented Oct 30, 2022 •

edited

Loading