Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix: None in "wait" Methods Causing Incorrectly Passing Tests #2823

Merged
merged 11 commits into from
Apr 19, 2024

Conversation

TillerBurr
Copy link
Contributor

@TillerBurr TillerBurr commented Mar 30, 2024

Closes #2818. This is my take on solving the issue. This method ignores the value prop if it is None. I think there is merit to adding a wait_for_value_to_equal, but this solves the immediate problem, while causing as little friction as possible.

As mentioned in #2818, there are also potential issues in the other methods in the dash.testing.wait namespace. Not sure if those cases need to be handled here.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Check if the value attribute is None and don't compare if it is.
    • Add tests to make sure the None isn't cast to a string.
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@TillerBurr
Copy link
Contributor Author

Tests are failing due to bad test that is fixed in #2816. Once that PR is merged and this merged into this PR, the tests will all pass.

@TillerBurr TillerBurr marked this pull request as ready for review April 7, 2024 03:37
@ndrezn ndrezn requested a review from emilykl April 15, 2024 15:14
with pytest.raises(TimeoutException) as err:
dash_duo.wait_for_contains_text("#content", "None", timeout=1.0)

assert (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

with pytest.raises(TimeoutException) as err:
dash_duo.wait_for_contains_text("#value-item", "None", timeout=1.0)

assert (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@emilykl
Copy link
Contributor

emilykl commented Apr 15, 2024

I like this solution. Need to think through possible consequences though as this is technically a breaking change in Dash Testing.

emilykl and others added 4 commits April 15, 2024 18:23
Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @TillerBurr -- thanks for the contribution! ✨

There's some failing tests I want to investigate but I'm pretty sure they're not related to your changes.

@T4rk1n T4rk1n merged commit 93b513d into plotly:dev Apr 19, 2024
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Dash Testing: wait_for_text_to_equal may incorrectly succeed when used with text "None"
3 participants