Exception in question #10

MarcelWilson · 2022-06-28T19:05:49Z

Addresses #9
Requires ScreenPyHQ/screenpy#22

… be passed along to the hamcrest based tests as reason for failure.

MarcelWilson · 2022-06-28T19:07:20Z

@perrygoy I'm not sure how you want to address versioning. I updated setup.py to require screenpy 4.0.2 with the assumption that is the next version and that PR ScreenPyHQ/screenpy#22 will be merged.

perrygoy · 2022-07-05T01:53:50Z

@perrygoy I'm not sure how you want to address versioning. I updated setup.py to require screenpy 4.0.2 with the assumption that is the next version and that PR ScreenPyHQ/screenpy#22 will be merged.

I think that will make sense, as nothing will be affected until after we release. We'll probably just need to make sure that screenpy v4.0.2 gets released first, or at least very soon after.

perrygoy

Looks good to me!

I had one small comment about the caught_exception typing, which probably will get cleared up with the comment i made on the other PR.

screenpy_selenium/questions/element.py

…rm to protocols

…e *right* one this time)

perrygoy

I have one very small action item for you, if you choose to do it (i will need to re-review). If you don't want to, i can sneak in the describe methods when i bump the version before deploying.

perrygoy · 2022-07-17T02:43:29Z

screenpy_selenium/actions/chain.py


        for action in self.actions:
-            if "add_to_chain" not in dir(action):
+            if not isinstance(action, Chainable):


So much more readable.

perrygoy · 2022-07-17T02:44:42Z

screenpy_selenium/questions/element.py

        the_actor.should(See.the(Element(WELCOME_BANNER), IsVisible()))
    """

+    caught_exception: Optional[TargetingError]


We were talking about this on Discord, and i do really like how we can be extremely explicit here about what kind of exceptions we're expecting to handle in the actual Questions.

perrygoy · 2022-07-17T02:46:09Z

tests/test_actions.py

+    def test_implements_protocol(self):
+        r = SaveConsoleLog("")
+        assert isinstance(r, Performable)


If you don't mind, can you add the describe method to SaveConsoleLog and SaveScreenshot? I forgot to add those methods when i added the Actions.

perrygoy

I appreciate all the extra testing, and the reinforcement of the existing tests as well! Thanks for doing this, @bandophahita.

Like the other PR, i had a few small things that i think we should address before merging. I promise to re-review faster this time!

perrygoy · 2022-07-20T20:33:11Z

mypy.ini

+show_error_codes = True
+exclude = (?x)(
+          setup\.py
+          ;          | tests/
+          | docs/
+          )
+
+[mypy-screenpy.*]
 disallow_untyped_defs = True
+
+[mypy-tests.*]
+ignore_missing_imports = True


What do these changes do, and why are they necessary? Also, do we need that commented line 5?

In most cases these settings do nothing for contributers since the pre-commit calls pylint and flake8 to inspect the code. It's really only useful for anyone that want to only run mypy manually on their end for the entire project. Handy, but not actually required by the codebase. I just found I was constantly adding them when I went to inspect the code manually locally.

Here is what the settings do:

show_error_code : includes the actual error code when running mypy rather than only the message. I dunno why mypy doesn't do this by default.

exclude : ignores the setup.py and the docs folder when running mypy manually against the entire project.

I initially ignored the tests folder then later commented it out. I can remove the comment.

ignore_missing_imports : ignores packages that don't have a typeshed to refer to.

Forgot to mention moving disallow_untyped_defs down into [mypy-screenpy.*]. This enforces the mypy rule specifically where it should be, which is in the screenpy directory rather than the entire project. This felt consistent with flake8 and pylint ignoring the tests also.

perrygoy · 2022-07-20T20:33:27Z

screenpy_selenium/actions/save_console_log.py

+    def describe(self) -> str:
+        """Describe the Action in present tense."""
+        return f"Save browser console log as {self.filename}"


Thanks again for catching and fixing these!

perrygoy · 2022-07-20T20:36:19Z

tests/test_actions.py

+from selenium.common.exceptions import WebDriverException
+from selenium.webdriver.common.action_chains import ActionChains
+from selenium.webdriver.common.keys import Keys
+from selenium.webdriver.remote.webelement import WebElement
+from selenium.webdriver.support import expected_conditions as EC


if we're organizing these by the system -> framework -> module -> local ordering, the Selenium imports should go up by the base screenpy imports.

That raises the question of should the pre-commit hooks handle that for us? isort would likely do this for us but pre-commit doesn't touch the tests.

Yes, isort does. It's been a while coming, i've been meaning to split out the configs for each of our precommit things.

perrygoy · 2022-08-04T16:01:05Z

tests/test_actions.py

+    target = mock.create_autospec(Target, instance=True)
+    element = mock.create_autospec(WebElement, instance=True)


Very nice. :D

perrygoy · 2022-08-04T16:02:08Z

tests/unittest_protocols.py

+class Describable(Protocol):
+    def describe(self) -> str:
+        ...


We won't have access to this Protocol here in screenpy_selenium until the newer version of ScreenPy is released. Should we add a comment here to remind ourselves to remove this one once we have the other available?

That's a good question. I don't know what the right answer is here. But, probably?

perrygoy · 2022-08-04T16:02:54Z

tests/test_resolutions.py

-    @mock.patch("selenium.webdriver.remote.webelement.WebElement", spec=WebElement)
-    def test_matches_a_clickable_element(self, element):
+    def test_matches_a_clickable_element(self):
+        element = mock.create_autospec(WebElement, instance=True)


Should we have a get_mock_element function for this?

We could. I didn't see it reducing the number of lines of code, but I don't think it would hurt anything.

perrygoy

Thanks again for all this! :D

ScreenPyHQ#9 adding the ability for exceptions raised in questions to…

631e38a

… be passed along to the hamcrest based tests as reason for failure.

perrygoy previously approved these changes Jul 5, 2022

View reviewed changes

screenpy_selenium/questions/element.py Outdated Show resolved Hide resolved

ScreenPyHQ#10 utilizing the runtime_checkable to verify objects confo…

5fb81aa

…rm to protocols

bandophahita dismissed perrygoy’s stale review via 5fb81aa July 12, 2022 17:08

ScreenPyHQ#10 screenpy renamed protocol (hopefully we've landed on th…

efd71ef

…e *right* one this time)

perrygoy previously approved these changes Jul 17, 2022

View reviewed changes

Adding missing describe methods

dddbb7a

bandophahita dismissed perrygoy’s stale review via dddbb7a July 19, 2022 16:52

bandophahita added 8 commits July 19, 2022 19:27

Adding coverage

a339e47

Adding describable protocol checks

79273c1

Missed a check for describable

44578fd

Forgot to spec out the other mocked drivers

e65814a

adding spec to mock object

f09bf60

cleaning up mock objects and using autospec where possible.

886172c

using create_autospec in cases where spec was used.

fe3e986

ensuring mocked objects that should be instances are set accordingly.

39b4f4f

bandophahita requested a review from perrygoy July 21, 2022 21:23

perrygoy reviewed Aug 4, 2022

View reviewed changes

updates based on comments in ScreenPyHQ#10

08851a4

perrygoy approved these changes Aug 5, 2022

View reviewed changes

bandophahita merged commit 73a9c7f into ScreenPyHQ:trunk Aug 9, 2022

MarcelWilson deleted the exception_in_question branch September 15, 2022 14:19

MarcelWilson mentioned this pull request Jan 23, 2025

questions missing caught_exception logic #62

Open

		target = mock.create_autospec(Target, instance=True)
		element = mock.create_autospec(WebElement, instance=True)

Exception in question #10

Exception in question #10

Uh oh!

Conversation

MarcelWilson commented Jun 28, 2022

Uh oh!

MarcelWilson commented Jun 28, 2022

Uh oh!

perrygoy commented Jul 5, 2022

Uh oh!

perrygoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

perrygoy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perrygoy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bandophahita Aug 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perrygoy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bandophahita Aug 4, 2022 •

edited

Loading