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

Minor cleanups to integration tests #6734

Merged
merged 2 commits into from Nov 8, 2018

Conversation

Projects
None yet
4 participants
@CMLivingston
Contributor

CMLivingston commented Nov 7, 2018

Minor comment cleanups to integration tests in the Python backend.

@stuhood stuhood requested a review from cosmicexplorer Nov 7, 2018

@wisechengyi

Thanks for the clarification!

@CMLivingston

This comment has been minimized.

Contributor

CMLivingston commented Nov 7, 2018

This has changed from the time of your accept @wisechengyi.

@stuhood

The title is stale.

@@ -122,21 +122,13 @@ def test_ctypes_third_party_integration(self):
self.assert_success(pants_run)
self.assertIn('Test worked!\n', pants_run.stdout_data)
# Test cached run.

This comment has been minimized.

@stuhood

stuhood Nov 7, 2018

Member

To test a cached run, you could use something like: https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/backend/jvm/tasks/jvm_compile/test_dep_exports_integration.py#L35

ie, run pants twice in the same temporary workdir.

This comment has been minimized.

@CMLivingston

CMLivingston Nov 7, 2018

Contributor

Ack, will add.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Nov 7, 2018

Contributor

Is that test checking for a cached run by checking whether the output contains the target spec (the last line of that method)? Where is that message coming from? How does that technique compare to the use of self.mock_buildroot() as used in another method also named test_invalidation()?

This comment has been minimized.

@CMLivingston

CMLivingston Nov 7, 2018

Contributor

@stuhood after reviewing the conan code further, I see that this test is unnecessary because really the only thing that needs to be tested is whether the invalidation check does what it is supposed to.

@CMLivingston CMLivingston changed the title from Add clean-alls where needed and helpful comments to Minor cleanups to integration tests Nov 7, 2018

@cosmicexplorer

Thanks for cleaning this up!

@stuhood stuhood merged commit 77edbcd into pantsbuild:master Nov 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

wisechengyi added a commit to wisechengyi/pants that referenced this pull request Nov 12, 2018

Minor cleanups to integration tests (pantsbuild#6734)
Minor comment cleanups to integration tests in the Python backend.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment