refactor(chore) Add mypy static type checking #321

cashall-0 · 2024-10-21T22:22:04Z

Because

We want to enhance the project to use static typing

This commit

Fixes all objects and methods to be type-checked
Update test files to be type-checked
Add a command to the Makefile to run the static type checking
Add a test to the github actions workflow to run the type checker on pull requests.

Fixes #314

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 · 2024-10-22T19:25:38Z

Hi @b4handjr , the CI type checking is failing because it seems not to be installing mypy. I added mypy to the group dev dependencies, is this appropriate?

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr · 2024-10-22T22:30:12Z

Makefile

+typecheck:
+	poetry run mypy .


Looks like you forgot to add install_dependencies here. Look at the lines above for a clue.

Thanks, it now installs mypy by adding install_dependencies, I was missing that.

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 · 2024-10-23T14:54:57Z

Hi @b4handjr, please can you look into my pr to recommend necessary changes

b4handjr · 2024-10-24T14:23:41Z

Looking at the build on github actions, it seems there is a failing type check in the docs/ directory, can you please set the type checker to ignore that directory? I also see some failures within tests/webserver.py.

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 · 2024-10-24T15:26:52Z

Looking at the build on github actions, it seems there is a failing type check in the docs/ directory, can you please set the type checker to ignore that directory? I also see some failures within tests/webserver.py.

I have set mypy to ignore the doc folder, and also cleared the webserver error by removing the deprecated import.

b4handjr · 2024-10-24T18:45:29Z

Looking at the build on github actions, it seems there is a failing type check in the docs/ directory, can you please set the type checker to ignore that directory? I also see some failures within tests/webserver.py.

I have set mypy to ignore the doc folder, and also cleared the webserver error by removing the deprecated import.

Great thanks, I will take a look

b4handjr · 2024-10-29T15:47:36Z

Hello @cashall-0, please pull main and update accordingly, I merge the fix for the failing tests. Thanks!

cashall-0 · 2024-10-29T18:40:58Z

Hello @cashall-0, please pull main and update accordingly, I merge the fix for the failing tests. Thanks!

Alright, I will get to it.

…ature/add-static-typing

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 · 2024-10-30T12:45:49Z

Hello @cashall-0, please pull main and update accordingly, I merge the fix for the failing tests. Thanks!

I have pulled and updated accordingly, do let me know whatever needs to change as you review.

b4handjr · 2024-10-30T15:29:48Z

Hello @cashall-0, please pull main and update accordingly, I merge the fix for the failing tests. Thanks!

I have pulled and updated accordingly, do let me know whatever needs to change as you review.

Thanks! I'll give it a look today

b4handjr

Thanks for working on this. I left some comments, I also noticed some of your new files don't have new lines at the end of them.

b4handjr · 2024-10-25T19:42:26Z

foxpuppet/expected.py

+    if TYPE_CHECKING:
+        from foxpuppet.windows import BrowserWindow  # Import for static typing


What's this for?

It keeps showing errors of cyclic dependency, like two classes importing from each other. so I imported for just the typechecking.
The current import is done inside the method.
example:
ImportError: cannot import name 'BrowserWindow' from partially initialized module 'foxpuppet.windows' (most likely due to a circular import) (/root/outreachy/moxilla/FoxPuppet/foxpuppet/windows/init.py)

I did some research on this and this pattern seems to be the accepted way

tests/test_notifications.py

b4handjr · 2024-10-25T19:51:33Z

foxpuppet/windows/base.py

@@ -26,7 +29,7 @@ def __init__(self, selenium, handle):
        self.wait = WebDriverWait(self.selenium, timeout=10)


Should this have a type?

it can,I assigned type WebDriverWait

b4handjr · 2024-10-31T14:23:12Z

foxpuppet/windows/browser/notifications/addons.py

@@ -21,7 +22,7 @@ class AddOnInstallConfirmation(BaseNotification):
    """Add-on install confirmation notification."""

    @property
-    def addon_name(self):
+    def addon_name(self) -> str | Any:


Why is this also Any?

done, changed to str

b4handjr · 2024-10-31T14:28:00Z

foxpuppet/windows/manager.py

+    from foxpuppet.windows import BrowserWindow
+


Could this import not go at the top?

b4handjr · 2024-10-31T14:33:18Z

tests/conftest.py

    """First Firefox browser window opened."""
    return foxpuppet.browser


 @pytest.fixture
-def firefox_options(firefox_options):
+def firefox_options(firefox_options: Any) -> Any:


You can use this, from selenium.webdriver.firefox.options import Options as FirefoxOptions

Done, but I had problems with these after using the type FireFoxOptions
if os.getenv("MOZREGRESSION_BINARY"): firefox_options.binary = os.getenv("MOZREGRESSION_BINARY") # type: ignore firefox_options.log.level = "trace" # type: ignore

that's why I used type ignore. Can you please look at this. In line 54 - 56.

b4handjr · 2024-10-31T14:34:51Z

tests/test_notifications.py

+def test_open_close_notification(
+    browser: BrowserWindow, blocked_notification: AddOnInstallBlocked
+) -> BaseNotification:


This is a test, it shouldn't return anything

b4handjr · 2024-10-31T14:35:34Z

tests/webserver.py

@@ -32,7 +33,7 @@ def __init__(self, host="", port=8000):
        self.thread = threading.Thread(target=self.server.serve_forever, daemon=True)

    @property
-    def host(self):
+    def host(self) -> Any | str:


Is this really Any or string? Why not just a string or int?

it has the types "str | bytes | bytearray" leaving it at "str" or "str | int" would show an error. But on tracing the variables and method calls it shows type assigned as "_RetAddress" which was used as a type alias for "Any".

Ah okay gotcha, thanks for that explanation.

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr

Left some of my thoughts. There are also some files that are missing a new line

b4handjr · 2024-11-04T15:49:14Z

tests/conftest.py

    if os.getenv("MOZREGRESSION_BINARY"):
-        firefox_options.binary = os.getenv("MOZREGRESSION_BINARY")
-    firefox_options.log.level = "trace"
+        firefox_options.binary = os.getenv("MOZREGRESSION_BINARY")  # type: ignore


Lets get rid of this if statement, we shouldn't need this MOZREGRESSION_BINARY stuff

Okay, I have removed the check and binary assignment.

b4handjr · 2024-11-04T15:52:06Z

tests/conftest.py

-        firefox_options.binary = os.getenv("MOZREGRESSION_BINARY")
-    firefox_options.log.level = "trace"
+        firefox_options.binary = os.getenv("MOZREGRESSION_BINARY")  # type: ignore
+    firefox_options.log.level = "trace"  # type: ignore


Could this be str?

log.level is actually of the type str, but the default value assigned to the log.level in options.py is None, so this somehow is interfering with type check making it ask for a type None.

b4handjr · 2024-11-04T15:58:47Z

tests/test_notifications.py

+def blocked_notification(
+    addon: Any, browser: BrowserWindow, webserver: WebServer, selenium: WebDriver
+) -> BaseNotification:


addon should have a type.

done, unbundled the class AddOn which was previously encapsulated in a method.

b4handjr · 2024-11-04T16:00:22Z

tests/test_notifications.py

+def test_confirm_addon_install(
+    addon, browser: BrowserWindow, confirmation_notification: AddOnInstallConfirmation
+) -> None:


addon should have a type.

b4handjr · 2024-11-04T16:00:38Z

tests/test_notifications.py

+def test_addon_install_complete(
+    addon, browser: BrowserWindow, complete_notification: AddOnInstallComplete
+) -> None:


addon should have a type.

tests/conftest.py

cashall-0 · 2024-11-05T10:59:31Z

Thanks for working on this. I left some comments, I also noticed some of your new files don't have new lines at the end of them.

I might be confused about what new files you are referring to, can you specify one of such?

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 · 2024-11-05T11:36:51Z

@b4handjr I'm trying to update my branch and push but there is a conflict in poetry.lock, the hash generated is different due to changes in project.toml. Which of these should I accept? my hash or the hash from main ? or is there a better way to resolve this?

b4handjr · 2024-11-05T16:11:11Z

@b4handjr I'm trying to update my branch and push but there is a conflict in poetry.lock, the hash generated is different due to changes in project.toml. Which of these should I accept? my hash or the hash from main ? or is there a better way to resolve this?

You could delete this file and merge in the new pyproject.toml file and just generate a new poetry.lock file, that should resolve the conflicts.

Let me know when you get that resolved and I'll take another look. It's looking good so far, thank you!

.github/workflows/linux_tests.yml

Makefile

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 · 2024-11-05T19:51:13Z

@b4handjr I'm trying to update my branch and push but there is a conflict in poetry.lock, the hash generated is different due to changes in project.toml. Which of these should I accept? my hash or the hash from main ? or is there a better way to resolve this?

You could delete this file and merge in the new pyproject.toml file and just generate a new poetry.lock file, that should resolve the conflicts.

Let me know when you get that resolved and I'll take another look. It's looking good so far, thank you!

While trying to resolve this, I noticed poetry version changed from 1.8.4 to 1.8.3 on the main. Is there a particular reason for the downgrade?

…ature/add-static-typing

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 · 2024-11-05T21:12:21Z

@b4handjr , I have tried to resolve the conflict and fixed from your recommendations. you can now take another look , thanks.

b4handjr · 2024-11-06T15:27:21Z

@b4handjr , I have tried to resolve the conflict and fixed from your recommendations. you can now take another look , thanks.

Thanks, I'll take a look but I think this is looking good

b4handjr

A few spots in the main modules that are missing types. I don't think we need to be too picky in the tests. Thanks for the work so far, this is just about done!

foxpuppet/windows/base.py

foxpuppet/windows/browser/notifications/addons.py

b4handjr · 2024-11-07T15:59:02Z

foxpuppet/windows/browser/notifications/base.py

@@ -31,15 +37,15 @@ def create(window, root):
            :py:class:`BaseNotification`: Firefox notification.

        """
-        notifications = {}
+        notifications: dict[Any, Any] = {}
        _id = root.get_property("id")


This could use a type

Done, it is of the type str but mypy wouldn't let my assign only that so I used suggested str | bool | WebElement | dict

tests/webserver.py

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr

Just a few final comments. I'd like to not use Any as much as possible, but there will be some places where we need it.

tests/test_notifications.py

b4handjr · 2024-11-12T15:26:39Z

foxpuppet/windows/browser/notifications/base.py

        """Retrieve the notification description."""
        if self.window.firefox_version >= 67:
            return self.root.find_element(By.CLASS_NAME, "popup-notification-description")
        return self.root.find_anonymous_element_by_attribute(
            "class", "popup-notification-description"
        )

-    def find_close_button(self):
+    def find_close_button(self) -> WebElement | Any:


It seems this could just a return a WebElement no?

Yes , but for this method and four others written this way
def find_close_button(self) -> WebElement | Any: """Retrieve the close button.""" if self.window.firefox_version >= 67: return self.root.find_element(By.CLASS_NAME, "popup-notification-closebutton") return self.root.find_anonymous_element_by_attribute("anonid", "closebutton")

for the second return statement here, I can't seem to find the implementation of the method find_anonymous_element_by_attribute() hence, the any for whatever the outcome.

foxpuppet/windows/base.py

foxpuppet/windows/browser/notifications/addons.py

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 added 2 commits October 21, 2024 23:16

add mypy dependency and configuration

e27e99a

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

fix makefile error

0b27a35

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 added 2 commits October 22, 2024 20:54

fixed methods and objects in foxpuppet/ expected, foxpuppet and region

b06405b

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

try to fix circular import in expected.py

1d9cea1

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr reviewed Oct 22, 2024

View reviewed changes

cashall-0 added 6 commits October 23, 2024 07:35

try to fix failing lint and makefile typecheck config

13846e5

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

add types to windows/ base , manager

0df87c9

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

add types to browser/ navbar, window

547d926

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

add types to notifications/ addons, base

c35155a

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

fix browser type errors

4c14d8d

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

Add types to test files

48635cc

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

excluded docs folderfrom typecheck and fixed errors in test/webserver

224f677

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr self-requested a review October 24, 2024 18:45

cashall-0 added 2 commits October 29, 2024 19:45

Merge branch 'main' of https://github.com/cashall-0/FoxPuppet into fe…

496d077

…ature/add-static-typing

fix webserver test any type error

2df5814

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr requested changes Oct 31, 2024

View reviewed changes

cashall-0 added 2 commits November 3, 2024 20:41

fix types from review comments

fa9ed4b

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

fix lint

3c8e814

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr requested changes Nov 4, 2024

View reviewed changes

change review recommended types

0d1cdf9

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr requested changes Nov 5, 2024

View reviewed changes

.github/workflows/linux_tests.yml Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

delete poetry.lock

f7832c4

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

cashall-0 added 2 commits November 5, 2024 20:56

Merge branch 'main' of https://github.com/cashall-0/FoxPuppet into fe…

7980c72

…ature/add-static-typing

add space to end of files and fix poetry_lock

38a0abf

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr requested changes Nov 7, 2024

View reviewed changes

more type check fixes from review

e1ce2b3

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

b4handjr self-requested a review November 12, 2024 15:15

b4handjr requested changes Nov 12, 2024

View reviewed changes

fix more types

56e6bf3

Signed-off-by: siyaka promise <siyakapromise@gmail.com>

		if TYPE_CHECKING:
		from foxpuppet.windows import BrowserWindow # Import for static typing

		@@ -26,7 +29,7 @@ def __init__(self, selenium, handle):
		self.wait = WebDriverWait(self.selenium, timeout=10)

refactor(chore) Add mypy static type checking #321

Are you sure you want to change the base?

refactor(chore) Add mypy static type checking #321

Conversation

cashall-0 commented Oct 21, 2024

cashall-0 commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cashall-0 commented Oct 23, 2024

b4handjr commented Oct 24, 2024

cashall-0 commented Oct 24, 2024

b4handjr commented Oct 24, 2024

b4handjr commented Oct 29, 2024

cashall-0 commented Oct 29, 2024

cashall-0 commented Oct 30, 2024

b4handjr commented Oct 30, 2024

b4handjr 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

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

cashall-0 Nov 4, 2024 • edited Loading

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

b4handjr 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

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

cashall-0 commented Nov 5, 2024

cashall-0 commented Nov 5, 2024

b4handjr commented Nov 5, 2024 • edited Loading

cashall-0 commented Nov 5, 2024

cashall-0 commented Nov 5, 2024

b4handjr commented Nov 6, 2024

b4handjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b4handjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cashall-0 Nov 4, 2024 •

edited

Loading

b4handjr commented Nov 5, 2024 •

edited

Loading