Skip to content

Commit

Permalink
REFACTOR: throw ConditionMismatch only on falsy exceptions
Browse files Browse the repository at this point in the history
  • Loading branch information
yashaka committed Jun 22, 2024
1 parent 91a191d commit 3d457ac
Show file tree
Hide file tree
Showing 8 changed files with 65 additions and 50 deletions.
12 changes: 7 additions & 5 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,13 @@ TODOs:

### TODO: not_ as callable object?

### TODO: should we raise InvalidCompare on _inverted too?

seems like currently we do raise, but cover with tests

### TODO: conditions can read options from config? commands/queries can read them?

especially relevant for have.texts to turn on/off ignoring invisible elements

## 2.0.0rc10: «copy&paste, frames, shadow & texts_like» (to be released on DD.05.2024)

Expand All @@ -130,9 +137,6 @@ TODOs:
### TODO: ENSURE composed conditions work as expected (or/and, etc.)

...
### TODO: should we raise InvalidCompare on _inverted too?

seems like currently we do raise, but cover with tests

### TODO: Consider renaming description to name for Condition, Match, Query, Command, etc.

Expand All @@ -144,8 +148,6 @@ seems like currently we do raise, but cover with tests

#### TODO: do we need positional actual and by args for Match?

### TODO: decide on _falsy_exceptions name

### Deprecated conditions

- `be.present` in favor of `be.present_in_dom`
Expand Down
41 changes: 17 additions & 24 deletions selene/core/condition.py
Original file line number Diff line number Diff line change
Expand Up @@ -839,26 +839,6 @@ def __init__(
actual: Lambda[E, R] | None = None,
by: Predicate[R] | None = None,
_inverted=False,
# TODO: better name for _falsy_exceptions?
# falsy means that it will become true on inverted
# i.e. such exceptions if inverted, will be considered as "truthy"
# the problem with such name is that if _inverted=False
# then any exception is a False in context of condition behavior,
# that simply throw an error as Falsy behavior
# maybe then better name would be:
# _truthy_exceptions_on_inverted = (AssertionError,)
# or
# _pass_on_inverted_when = (AssertionError,)
# or
# _pass_on_inverted_when = (AssertionError,)
# hm... but maybe, we actually would want to keep _falsy_exceptions
# but make it more generous... i.e. not throwing ConditionMismatch
# on any error that is not "falsy"... Make the logic the following:
# if error is in _falsy_exceptions, then raise ConditionMismatch
# (or pass on inverted)
# else just pass the error through... This will give an opportunity
# to decide whether to ignore some non-condition-mismatch errors
# inside wait.for_ ...
_falsy_exceptions: Iterable[Type[Exception]] = (AssertionError,),
):
# can be already stored
Expand All @@ -876,9 +856,16 @@ def __init__(
self.__actual = actual
self.__by = by
self.__test = (
ConditionMismatch._to_raise_if_not(self.__by, self.__actual)
ConditionMismatch._to_raise_if_not(
self.__by,
self.__actual,
_falsy_exceptions=_falsy_exceptions,
)
if self.__actual
else ConditionMismatch._to_raise_if_not(self.__by)
else ConditionMismatch._to_raise_if_not(
self.__by,
_falsy_exceptions=_falsy_exceptions,
)
)
self.__test_inverted = (
ConditionMismatch._to_raise_if_actual(
Expand Down Expand Up @@ -910,8 +897,14 @@ def as_inverted(entity: E) -> None:
test(
entity
) # called via test, not self.__test to make mypy happy :)
except Exception: # TODO: should we check only AssertionError here?
return
# todo: ensure covered with tests (we might not have the ones)
except Exception as reason:
is_falsy = any(
isinstance(reason, exception) for exception in _falsy_exceptions
)
if is_falsy:
return
raise reason
raise ConditionMismatch()

self.__test_inverted = as_inverted
Expand Down
28 changes: 17 additions & 11 deletions selene/core/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,10 @@ def _to_raise_if_not(
actual: Optional[Callable[[E], E | R]] = None,
*,
_inverted: Optional[bool] = False,
# TODO: should we rename it to _exceptions_as_truthy_on_inverted?
# todo: should we rename it to _exceptions_as_truthy_on_inverted?
# or just document this in docstring?
# – seems like no, because now falsy are always falsy:)
# as we started to differentiate raising falsy from raising non-falsy
_falsy_exceptions: Iterable[Type[Exception]] = (AssertionError,),
):
@functools.wraps(by)
Expand Down Expand Up @@ -189,15 +191,16 @@ def describe_error(error):
try:
actual_to_test = actual(entity) if actual else entity
except Exception as reason:
if _inverted and any(
is_falsy = any(
isinstance(reason, exception) for exception in _falsy_exceptions
):
)
if _inverted and is_falsy:
return
# todo: do we even need this prefix?
# raise cls(f'Unable to get actual to match:\n{describe_error(reason)}')
# TODO: should we wrap as ConditionMismatch only those errors that are in _false_exceptions?
# seems like yes...
raise cls(describe_error(reason)) from reason
# only _falsy_exceptions can be considered as "condition mismatch"
# others should signify the "total comparison is invalid in this case" failure
# todo: should we rebuild reason if it's not is_falsy
# by applying describe_error like we did further below?
raise (cls(describe_error(reason)) if is_falsy else reason) from reason

answer = None
try:
Expand All @@ -208,12 +211,15 @@ def describe_error(error):
# – no, we should not, we should keep it here,
# because this is needed for the inverted case
except Exception as reason:
if _inverted and any(
is_falsy = any(
isinstance(reason, exception) for exception in _falsy_exceptions
):
)
if _inverted and is_falsy:
return
# answer is still None
raise cls(
raise (cls if is_falsy else reason.__class__)(
# todo: should we still remove stacktrace from reason
# if it's not is_falsy?
f'{describe_error(reason)}:'
f'\n{describe_not_match(actual_to_test)}'
) from reason
Expand Down
6 changes: 6 additions & 0 deletions selene/core/wait.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ def __init__(
_decorator: (
Callable[[Wait[E]], Callable[[Callable[..., R]], Callable[..., R]]] | None
) = None,
# TODO: should not we add here ignore_exceptions?
# (called as _falsy_exceptions in Condition init)
# and then tune it depending on context,
# e.g. in should we need to ignore only ConditionMismatch errors
# in commands we probably need to ignore a low more of WebDriverExceptions
# and so on...
):
self.entity = entity
self._timeout = at_most
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def test_should_be_enabled__passed_and_failed(session_browser):
'Timed out after 0.1s, while waiting for:\n'
"browser.element(('css selector', '#absent')).is enabled\n"
'\n'
'Reason: ConditionMismatch: Message: no such element: Unable to locate '
'Reason: NoSuchElementException: Message: no such element: Unable to locate '
'element: {"method":"css selector","selector":"#absent"}\n'
' (Session info:' # ' chrome=126.0.6478.62); For documentation on this error, '
# 'please visit: '
Expand Down Expand Up @@ -148,7 +148,7 @@ def test_should_be_not_enabled__passed_and_failed(
'Timed out after 0.1s, while waiting for:\n'
"browser.element(('css selector', '#absent')).is not (enabled)\n"
'\n'
'Reason: ConditionMismatch: Message: no such element: Unable to locate '
'Reason: NoSuchElementException: Message: no such element: Unable to locate '
'element: {"method":"css selector","selector":"#absent"}\n'
' (Session info:' # ' chrome=126.0.6478.62); For documentation on this error, '
# 'please visit: '
Expand Down Expand Up @@ -213,7 +213,7 @@ def test_should_be_disabled__passed_and_failed(
'Timed out after 0.1s, while waiting for:\n'
"browser.element(('css selector', '#absent')).is not (enabled)\n"
'\n'
'Reason: ConditionMismatch: Message: no such element: Unable to locate '
'Reason: NoSuchElementException: Message: no such element: Unable to locate '
'element: {"method":"css selector","selector":"#absent"}\n'
' (Session info:' # ' chrome=126.0.6478.62); For documentation on this error, '
# 'please visit: '
Expand Down Expand Up @@ -276,7 +276,7 @@ def test_should_be_not_disabled__passed_and_failed(session_browser):
'Timed out after 0.1s, while waiting for:\n'
"browser.element(('css selector', '#absent')).is enabled\n"
'\n'
'Reason: ConditionMismatch: Message: no such element: Unable to locate '
'Reason: NoSuchElementException: Message: no such element: Unable to locate '
'element: {"method":"css selector","selector":"#absent"}\n'
' (Session info:' # ' chrome=126.0.6478.62); For documentation on this error, '
# 'please visit: '
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ def test_text_matching__regex_pattern__error__on_invalid_regex__with_ignorecase(
"browser.all(('css selector', 'li'))[0].has text matching (with flags "
're.IGNORECASE): *one*\n'
'\n'
'Reason: ConditionMismatch: nothing to repeat at position '
'Reason: error: nothing to repeat at position '
'0:\n'
'actual text: 1) One!!!\n'
'Screenshot: '
Expand All @@ -333,7 +333,7 @@ def test_text_matching__regex_pattern__error__on_invalid_regex__with_ignorecase(
"browser.all(('css selector', 'li'))[0].has no (text matching (with flags "
're.IGNORECASE): *one*)\n'
'\n'
'Reason: ConditionMismatch: nothing to repeat at position '
'Reason: error: nothing to repeat at position '
'0:\n'
'actual text: 1) One!!!\n'
'Screenshot: '
Expand All @@ -347,7 +347,7 @@ def test_text_matching__regex_pattern__error__on_invalid_regex__with_ignorecase(
"browser.all(('css selector', 'li'))[0].has no (text matching (with flags "
're.IGNORECASE): *one*)\n'
'\n'
'Reason: ConditionMismatch: nothing to repeat at position '
'Reason: error: nothing to repeat at position '
'0:\n'
'actual text: 1) One!!!\n'
'Screenshot: '
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ def test_should_be_present__via_inline_Match__passed_and_failed(session_browser)
'present',
actual=lambda element: element.locate(),
by=lambda actual: actual is not None,
_falsy_exceptions=(NoSuchElementException,),
)
)
pytest.fail('expected failure')
Expand Down Expand Up @@ -119,6 +120,7 @@ def test_should_be_present__via_inline_Match__passed_and_failed(session_browser)
query=lambda element: element.locate(),
by=lambda actual: actual is not None,
),
_falsy_exceptions=(NoSuchElementException,),
),
'absent',
).not_
Expand Down Expand Up @@ -198,7 +200,13 @@ def test_should_be_present__via_inline_Match__passed_and_failed(session_browser)
)
# - with by failure
try:
absent.should(Match('present', by=lambda element: element.locate() is not None))
absent.should(
Match(
'present',
by=lambda element: element.locate() is not None,
_falsy_exceptions=(NoSuchElementException,),
)
)
pytest.fail('expected failure')
except AssertionError as error:
assert (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def test_should_be_selected__passed_and_failed(session_browser):
assert (
"browser.element(('css selector', '#absent')).is selected\n"
'\n'
'Reason: ConditionMismatch: Message: no such element: Unable to locate '
'Reason: NoSuchElementException: Message: no such element: Unable to locate '
'element: {"method":"css selector","selector":"#absent"}\n'
' (Session info:' # ' chrome=126.0.6478.62); For documentation on this error, '
# 'please visit: '
Expand Down Expand Up @@ -157,7 +157,7 @@ def test_should_be_not_selected__passed_and_failed(
assert (
"browser.element(('css selector', '#absent')).is not (selected)\n"
'\n'
'Reason: ConditionMismatch: Message: no such element: Unable to locate '
'Reason: NoSuchElementException: Message: no such element: Unable to locate '
'element: {"method":"css selector","selector":"#absent"}\n'
' (Session info:' # ' chrome=126.0.6478.62); For documentation on this error, '
# 'please visit: '
Expand Down

0 comments on commit 3d457ac

Please sign in to comment.