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

Resolve a "FIXME" on why inspect.ismethod() did not work. #331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iulianmac
Copy link
Contributor

@iulianmac iulianmac commented Apr 13, 2022

What:

Debug why inspect.ismethod() did not work in cli.py and update steps in contribution guide on building the docker image for development.

Why:

Resolves a FIXME comment and updates development guide on building Docker image for dev.

How:

Executed all unit-tests.

Done the following small test:

import inspect

class C(object):
    def foo(self):
        pass
>>> callable(getattr(C, "foo")) # What is now in code
True
>>> inspect.ismethod(getattr(C, "foo")) # Probably what caused the "FIXME" comment
False
>>> inspect.ismethod(getattr(C(), "foo")) # Propsed fix
True

Probably because inspect.ismethod() Return True if the object is a bound method written in Python.

>>> C.foo
<function C.foo at 0x104aa6ca0>
>>> C().foo
<bound method C.foo of <__main__.C object at 0x104ab87f0>>

Risks:

Potential cases not covered by unit-tests today.

Checklist:

  • Added tests, if you've added code that should be tested (N/A)
  • Updated the documentation, if you've changed APIs (N/A)
  • Ensured the test suite passes
  • Made sure your code lints
  • Completed the Contributor License Agreement ("CLA")

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 13, 2022
@iulianmac iulianmac marked this pull request as draft April 13, 2022 18:53
@iulianmac iulianmac marked this pull request as ready for review April 13, 2022 19:06
@iulianmac
Copy link
Contributor Author

iulianmac commented Apr 13, 2022

Seems that CI is failing for Python3.6 but looks unrelated to my changes. I was able to reproduce the failing case from "main" branch as well with python3.6.

>       assert "passed, 4 errors" in result.stdout.str()
E       AssertionError: assert 'passed, 4 errors' in ''
E        +  where '' = <bound method LineMatcher.str of <_pytest.pytester.LineMatcher object at 0xffffac8979e8>>()
E        +    where <bound method LineMatcher.str of <_pytest.pytester.LineMatcher object at 0xffffac8979e8>> = <_pytest.pytester.LineMatcher object at 0xffffac8979e8>.str
E        +      where <_pytest.pytester.LineMatcher object at 0xffffac8979e8> = <RunResult ret=ExitCode.USAGE_ERROR len(stdout.lines)=0 len(stderr.lines)=5 duration=0.02s>.stdout

/root/src/TestSlide/pytest-testslide/tests/test_pytest_testslide.py:129: AssertionError

@iulianmac
Copy link
Contributor Author

I've put out #332 as an attempt to fix CI tests.

@facebook-github-bot
Copy link

@deathowl has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants