search

facebookincubator / ptr

Python Test Runner.
MIT License
284 stars 15 forks source link

Move tests out of install but include in sdist #99

Closed jayvdb closed 4 years ago

facebook-github-bot commented 4 years ago

Hi @jayvdb!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

cooperlees commented 4 years ago

Hi,

Thanks for the PR.

What problem are you trying to fix here? Or what is you goal so I can understand.

Cheers, Cooper

cooperlees commented 4 years ago

These files do exist in the sdist on PyPI:

cooper-mbp1:tmp cooper$ tar xvzf ptr-20.2.26.tar.gz
x ptr-20.2.26/
x ptr-20.2.26/CHANGES.md
x ptr-20.2.26/LICENSE
x ptr-20.2.26/MANIFEST.in
x ptr-20.2.26/PKG-INFO
x ptr-20.2.26/README.md
x ptr-20.2.26/ptr.egg-info/
x ptr-20.2.26/ptr.egg-info/PKG-INFO
x ptr-20.2.26/ptr.egg-info/SOURCES.txt
x ptr-20.2.26/ptr.egg-info/dependency_links.txt
x ptr-20.2.26/ptr.egg-info/entry_points.txt
x ptr-20.2.26/ptr.egg-info/top_level.txt
x ptr-20.2.26/ptr.py
x ptr-20.2.26/ptr_tests.py  <--- HERE
x ptr-20.2.26/ptr_tests_fixtures.py  <--- HERE
x ptr-20.2.26/setup.cfg
x ptr-20.2.26/setup.py
jayvdb commented 4 years ago

@cooperlees , setup.py installs these test modules into site-packages.

facebook-github-bot commented 4 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

jayvdb commented 4 years ago

c.f. https://build.opensuse.org/package/rdiff/home:jayvdb:branches:devel:languages:python/python-ptr?opackage=python-ptr&oproject=devel%3Alanguages%3Apython&rev=8 for the pending packaging changes for openSUSE. This patch is to simplify that test logic.

jayvdb commented 4 years ago

Please advise whether the CI failures are due to my PR or other issues. It is a bit hard to see the cause from the CI logs.

cooperlees commented 4 years ago

I have the tests included on purpose.

c.f. https://build.opensuse.org/package/rdiff/home:jayvdb:branches:devel:languages:python/python-ptr?opackage=python-ptr&oproject=devel%3Alanguages%3Apython&rev=8 for the pending packaging changes for openSUSE. This patch is to simplify that test logic.

I can’t view cause I’m not logged in and I don’t think I’ll ever need an OpenSUSE account. Can you paste the error or logic and how you’d like to simplify it. We might be able to meet in the middle.

Please advise whether the CI failures are due to my PR or other issues. It is a bit hard to see the cause from the CI logs

Looking @ https://github.com/facebookincubator/ptr/pull/99/checks?check_run_id=1103517938 We get no output, but I would imagine import failures.

CI has been fine on all commits before this so I am sure the CI is accurate here. https://github.com/facebookincubator/ptr/actions We’ve put a lot of effort into the CI. I’ll try run this manually and get an actionable error.

jayvdb commented 4 years ago

_generate_test_suite_cmd makes the assumption that config["test_suite"] is a module, and that it is installed, as PYTHONPATH isnt being set to include the source/distribution directory. That last bit is problematic. Seems easy enough to fix.

jayvdb commented 4 years ago

Now getting Cannot locate a.pyre_configurationfile in the current directory or any parent directories. I'll dig into it tmr. Unfortunately pyre isnt packaged for openSUSE yet (volunteer effort at https://build.opensuse.org/project/show/home:bnavigator:pyre) .

cooperlees commented 4 years ago

Hi, do you plan on finishing this? You're very close. I could try add the pyre config and then you can rebase. Must be a requirement in newer versions of pyre.

jayvdb commented 4 years ago

If you could do the pyre bit, that would be great. I'm still stuck on trying to get it installable via rpms. I can rebase after you get it working, assuming that it is a problem unrelated to this PR.

cooperlees commented 4 years ago

Ok, merged https://github.com/facebookincubator/ptr/pull/100 as pyre seems to have more issues that will take awhile to fix "nicely". So please rebase and see how you go.

jayvdb commented 4 years ago

I'm guessing this error on macOS is unrelated.

 Traceback (most recent call last):
  File "ptr.py", line 1121, in <module>
    main()  # pragma: no cover
  File "ptr.py", line 1112, in main
    args.system_site_packages,
  File "/Users/runner/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/asyncio/base_events.py", line 488, in run_until_complete
    return future.result()
  File "ptr.py", line 1012, in async_main
    system_site_packages,
  File "ptr.py", line 912, in run_tests
    system_site_packages=system_site_packages,
  File "ptr.py", line 749, in create_venv
    await _gen_check_output(install_cmd, timeout=timeout)
  File "ptr.py", line 438, in _gen_check_output
    (stdout, stderr) = await asyncio.wait_for(process.communicate(), timeout)
  File "/Users/runner/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/asyncio/tasks.py", line 362, in wait_for
    raise futures.TimeoutError()
concurrent.futures._base.TimeoutError
Using Python 3.6.12
cooperlees commented 4 years ago

Yup - This was happening on my PR this morning too. I give it 120 seconds to install the deps via PIP. More than enough time. Don't know why those MacOS instances as so damn slow.

cooperlees commented 4 years ago

Is it helpful if I upload a new version to PyPI for you? There is no functionality changes but I can cut a new release so the wheel and sdist don't install the tests.