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 clean-all from pants invocations in python_dist() integration testing + some other refactoring #6474

Merged
merged 4 commits into from Sep 10, 2018

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Sep 9, 2018

Problem

Resolves #6455.

Solution

  • Remove uses of clean-all between python_dist() runs to ensure we're not breaking incremental builds.
  • Rename a few tests so using pytest's -k is easier.
  • Add some TODOs.

@cosmicexplorer cosmicexplorer requested review from stuhood , jsirois and CMLivingston Sep 9, 2018

@jsirois

jsirois approved these changes Sep 9, 2018

Copy link
Member

jsirois left a comment

Assuming green looks good mod not using 'current' and killing complexity + TODO.

@@ -46,7 +44,8 @@ def test_pants_binary(self):
output = subprocess.check_output(pex).decode('utf-8')
self._assert_native_greeting(output)
# Check that we have exact one wheel output

This comment has been minimized.

@jsirois

jsirois Sep 9, 2018

Member

exactly ... and end with period.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 9, 2018

Contributor

Done in 4456105!

@@ -46,7 +44,8 @@ def test_pants_binary(self):
output = subprocess.check_output(pex).decode('utf-8')
self._assert_native_greeting(output)
# Check that we have exact one wheel output
self.assertEqual(len(glob.glob(wheel_glob)), 1)
single_wheel_output = assert_single_element(glob.glob(os.path.join(tmp_dir, '*.whl')))
self.assertTrue(os.path.basename(single_wheel_output).startswith('fasthello-1.0.0+'))

This comment has been minimized.

@jsirois

jsirois Sep 9, 2018

Member

If you use assertRegexpMatches you'll get a better error message.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 9, 2018

Contributor

Done in 4456105!

@@ -66,7 +65,7 @@ def test_invalidation(self):
with self.mock_buildroot(
dirs_to_copy=[self.fasthello_install_requires_dir]) as buildroot, buildroot.pushd():
run_target = lambda: self.run_pants_with_workdir(
command=['-ldebug', 'run', fasthello_run],
command=['run', fasthello_run],
workdir=os.path.join(buildroot.new_buildroot, '.pants.d'),
build_root=buildroot.new_buildroot,
extra_env={'PEX_VERBOSE': '9'},

This comment has been minimized.

@jsirois

jsirois Sep 9, 2018

Member

Consider killing these - they were added to debug as I recall.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 9, 2018

Contributor

Removed all the PEX_VERBOSE from the codebase, including a stray in the testprojects integration test (in 4456105).

pants_run = self.run_pants(command=command, config=pants_ini_config)
pants_run = self.run_pants(command=command, config={
'python-setup': {
'platforms': [self._get_current_platform_string()],

This comment has been minimized.

@jsirois

jsirois Sep 9, 2018

Member

Why not use 'current'? That shortcut has existed >5 years.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 9, 2018

Contributor

I added this + explanation because I wasn't sure why the platform string method existed. Removed in 4456105.

@@ -179,25 +178,26 @@ def test_pants_resolves_local_dists_for_current_platform_only(self):

def _get_current_platform_string(self):
return Platform.create().resolve_platform_specific({
# TODO: this will fail on everyone's laptop if they're not at 10.12.

This comment has been minimized.

@jsirois

jsirois Sep 9, 2018

Member

Do you have a more actionable TODO? What's the fix?

How about use 'current' as suggested above.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 9, 2018

Contributor

Using 'current' allows us to remove this method (in 4456105)!

@@ -158,6 +156,7 @@ def test_pants_resolves_local_dists_for_current_platform_only(self):
pex = os.path.join(tmp_dir, 'main.pex')
pants_ini_config = {
'python-setup': {
# TODO: explain why we need 'this-platform-does_not-exist' here.

This comment has been minimized.

@jsirois

jsirois Sep 9, 2018

Member

Can you knock this out now?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 9, 2018

Contributor

Took a stab at answering in d5f00f9.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 9, 2018

Contributor

(it was left as a todo because I didn't know the answer)

cosmicexplorer added some commits Sep 9, 2018

@stuhood

stuhood approved these changes Sep 9, 2018

@cosmicexplorer cosmicexplorer merged commit 352c5ed into pantsbuild:master Sep 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment