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

Remove PYTHONPATH hax when running tests #170

Merged
merged 1 commit into from
Jan 21, 2016
Merged

Remove PYTHONPATH hax when running tests #170

merged 1 commit into from
Jan 21, 2016

Conversation

asottile
Copy link
Contributor

This probably requires some explaining (and may not be entirely complete).

Currently tox sets PYTHONPATH=paasta_tools/ which is entirely unrealistic in a real running situation, writing code which depends on this will pass under test but fail miserably when run in an integration scenario.

I also had to change py.test to python -m pytest. The former puts the bin/ directory on the pythonpath.

I removed the line in tox and then fixed all the things that broke because of it.

@solarkennedy
Copy link
Contributor

This is pretty epic, and looks great, but the itests are failing.
We run the itests (behave) in docker. It is currently failing with:

paasta_itests_inside_container runtests: commands[0] | behave --no-capture
Traceback (most recent call last):
  File "../../tmp/bin/behave", line 9, in <module>
    load_entry_point('behave==1.2.4', 'console_scripts', 'behave')()
  File "/tmp/local/lib/python2.7/site-packages/behave/__main__.py", line 111, in main
    failed = runner.run()
  File "/tmp/local/lib/python2.7/site-packages/behave/runner.py", line 659, in run
    return self.run_with_paths()
  File "/tmp/local/lib/python2.7/site-packages/behave/runner.py", line 664, in run_with_paths
    self.load_hooks()
  File "/tmp/local/lib/python2.7/site-packages/behave/runner.py", line 623, in load_hooks
    exec_file(hooks_path, self.hooks)
  File "/tmp/local/lib/python2.7/site-packages/behave/runner.py", line 306, in exec_file
    execfile(filename, globals, locals)
  File "/work/paasta_itests/environment.py", line 19, in <module>
    from itest_utils import wait_for_marathon
  File "/work/paasta_itests/itest_utils.py", line 21, in <module>
    from paasta_tools.utils import timeout
ImportError: No module named paasta_tools.utils
ERROR: InvocationError: '/tmp/bin/behave --no-capture'

Does behave need the same treatment that py.test got?

The general itests got:

Double requirement given: mock==1.0.1 (from -r requirements-dev.txt (line 6)) (already in mock==1.0.1, name='mock')

Which I think is because the testenv:paasta_itests_inside_container env got missed on this refactor.

@asottile
Copy link
Contributor Author

I'll look into these tomorrow :)

Does the overall idea seem sound?

@solarkennedy
Copy link
Contributor

In general this sounds good, not having the extra environment variable means less magic, at the costs of more words (more explicit) lines in the test files, seems fine.

Thank you for doing all the cleaning, it looks like there is a lot of loose ends. Here is what is failing now:

general_itests/steps/deployments_json_steps.py:61:121: E501 line too long (134 > 120 characters)
general_itests/steps/deployments_json_steps.py:85:121: E501 line too long (130 > 120 characters)

I guess we were not running flake on that before?

solarkennedy added a commit that referenced this pull request Jan 21, 2016
Remove PYTHONPATH hax when running tests
@solarkennedy solarkennedy merged commit 964719c into Yelp:master Jan 21, 2016
@solarkennedy
Copy link
Contributor

EPIC. Thanks a ton @asottile.

@asottile asottile deleted the no_pythonpath_hax branch January 21, 2016 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants