Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Update visibility_tests.py to verify that "_" is not overwritten when calling "is_displayed()" #12705

Closed
wants to merge 2 commits into from

Conversation

mdmintz
Copy link
Contributor

@mdmintz mdmintz commented Sep 7, 2023

Update visibility_tests.py to verify that _ is not overwritten when calling is_displayed().

This is a test for #12704, but in a separate PR to follow TDD principles. Currently, there is a bug in 4.12.0 (#12659), and this test should catch that (and currently fail because of it). Once #12704 is merged, this test should pass.

Types of changes

  • Add a test (TDD-style)

Checklist

  • I have read the contributing document.
  • I have added a test.

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.48%. Comparing base (af49a5e) to head (dadedf7).
Report is 1 commits behind head on trunk.

❗ Current head dadedf7 differs from pull request most recent head 72fa2cc. Consider uploading reports for the commit 72fa2cc to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #12705   +/-   ##
=======================================
  Coverage   58.48%   58.48%           
=======================================
  Files          86       86           
  Lines        5270     5270           
  Branches      220      220           
=======================================
  Hits         3082     3082           
  Misses       1968     1968           
  Partials      220      220           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@titusfortner titusfortner left a comment

Choose a reason for hiding this comment

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

I don't believe in running integration tests that verify something unusual doesn't happen just because there was once a bug. Testing for a generalized problem in JS unit tests would be preferred, but I think that would require a better understanding of what we want to do there and how to do it correctly and how to avoid doing it incorrectly, which I'm not sure we know.

@titusfortner titusfortner added C-py and removed python labels Oct 22, 2023
mdmintz added 2 commits March 25, 2024 16:52
Update visibility_tests.py to verify that "_" is not overwritten
Update visibility_tests.py (Linter changes)
@diemol
Copy link
Member

diemol commented Mar 25, 2024

The bug was fixed, but this test is failing. I am not sure what the intention was. I will close this for now and you can send another PR if you still want to have this test in trunk.

@diemol diemol closed this Mar 25, 2024
@mdmintz mdmintz deleted the patch-2 branch March 25, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants