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

[not-an-iterable] Add regression test for uninferable instances #9264

Merged
merged 1 commit into from
Nov 26, 2023

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

While investigating #9253, I found that the regression test for suppressing not-an-iterable for Uninferable instances was not testing what it intended. Inference had improved over eight years, and we were testing an Instance instead, not Uninferable.

Refs #9253

@jacobtylerwalls jacobtylerwalls added tests Skip news πŸ”‡ This change does not require a changelog entry labels Nov 25, 2023
Copy link

codecov bot commented Nov 25, 2023

Codecov Report

Merging #9264 (fc83c51) into main (b468e5b) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9264   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files         173      173           
  Lines       18761    18761           
=======================================
  Hits        17975    17975           
  Misses        786      786           

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Are we inferring successfully that something is Iterable from a module we can't import because the namekf the class is 'Iterable' ? This would fail on deceiving code, right ? (but on the other hand it's probably working very well on Real World code)

@jacobtylerwalls
Copy link
Member Author

From the comment, I took the motivation for the test to be testing an Uninferable. We weren't getting that. We were getting <Instance of blahblah.MyClass>, and testing that. We can keep the existing test if you like, I thought it was duplicative with the other tests in this module. The failed import is a red herring; it has nothing to do with the test, since we get <Instance of blahblah.MyClass> whether the import succeeds or not.

@jacobtylerwalls
Copy link
Member Author

Oh! The bases being able to be inferred actually does matter. I'll just keep both tests (and improve the comment).``

@jacobtylerwalls jacobtylerwalls changed the title [not-an-iterable] Improve regression test for Uninferable [not-an-iterable] Add regression test for Uninferable Nov 26, 2023
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thanks for investigating πŸ‘

@jacobtylerwalls
Copy link
Member Author

Rebased for the docs check fix

@jacobtylerwalls jacobtylerwalls changed the title [not-an-iterable] Add regression test for Uninferable [not-an-iterable] Add regression test for uninferable instances Nov 26, 2023
@Pierre-Sassoulas
Copy link
Member

Nice github enhancement I just noticed: rebase + force push don't trigger the need for a new approving review !

@jacobtylerwalls
Copy link
Member Author

πŸ™ https:/orgs/community/discussions/12876#discussioncomment-7445850

@jacobtylerwalls jacobtylerwalls enabled auto-merge (squash) November 26, 2023 13:56
@jacobtylerwalls jacobtylerwalls merged commit 56a9657 into main Nov 26, 2023
26 checks passed
@jacobtylerwalls jacobtylerwalls deleted the not-an-iterable-uninferable branch November 26, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip news πŸ”‡ This change does not require a changelog entry tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants