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

fix/integration test for pants_requirement() #5457

Merged

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Feb 12, 2018

Problem

It's not possible to use pants_requirement() to stand in for python_requirement_library(requirements=[python_requirement('pantsbuild.pants==1.5.0.dev3')]) in a separate repo. This defeats the purpose of pants_requirement(), which expands into an == requirement on the pants version denoted in the repo's pants.ini. The error is:

u"There is no type registered for alias <class 'pants.backend.python.targets.python_requirement_library.PythonRequirementLibrary'>"

Solution

ParseContext#create_object() accepts an alias as the first argument, not the class itself. Changing PythonRequirementLibrary to 'python_requirement_library' in src/python/pants/backend/python/pants_requirement.py allows pants_requirement() to be used as documented. Integration tests were added in tests/python/pants_test/backend/python{test_pants_requirement_integration,/tasks/test_setup_py_integration}.py.

@cosmicexplorer cosmicexplorer changed the title Dmcclanahan/fix pants requirement fix/integration test for pants_requirement() Feb 12, 2018
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -0,0 +1,15 @@
# coding=utf-8
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
Copy link
Member

Choose a reason for hiding this comment

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

2018

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has now been fixed.

@@ -0,0 +1 @@
pants_requirement()
Copy link
Member

Choose a reason for hiding this comment

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

Should this... have an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily, and not needed for this purpose. See test_pants_requirement.py for an example of providing a name and not.

from pants_test.base_test import BaseTest


class PantsPluginPantsRequirementTest(BaseTest):
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear that this is testing what it is testing... maybe an integration test that runs some pants command on the target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, this was completely removed.

'tests/python/pants_test:test_infra',
'testprojects/pants-plugins/3rdparty/pants',
],
tags={'integration'},
Copy link
Member

Choose a reason for hiding this comment

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

This isn't currently an integration test, but maybe it should be? See below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out here too -- this was removed as well.

@cosmicexplorer
Copy link
Contributor Author

Travis is failing, and I need to review ci.sh -- I'm not immediately sure how to make testprojects/tests/python/test_pants_plugin/BUILD only visible to the tests I want it to be. I could/should probably name it something else and just copy it to a file named BUILD -- I'll look at this later today.

@stuhood
Copy link
Member

stuhood commented Feb 13, 2018

Hm, yea: if you need a broken BUILD file you'd need to rename it temporarily inside the test. See

with self.file_renamed(os.path.join(prefix, 'cycle1'), 'TEST_BUILD', 'BUILD'):
for example.

he never leaves a single ring
that's why gryffindors all sing
stu hood is our king
@stuhood
Copy link
Member

stuhood commented Feb 14, 2018

Thanks!

@stuhood stuhood merged commit f55260a into pantsbuild:master Feb 14, 2018
@stuhood
Copy link
Member

stuhood commented Feb 24, 2018

It looks like this test is going to fail during between "bumping the version" and actually releasing the new bumped version. I'm seeing it fail #5513 for that reason (which bumps the version to 1.5.0.dev4, which is not yet published.

Can you adjust the test to use a stable version of pants?

@stuhood stuhood mentioned this pull request Feb 24, 2018
stuhood pushed a commit that referenced this pull request Feb 24, 2018
Landing with red CI due to #5457.
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.

2 participants