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

Deprecate --fast option #8970

Merged
merged 4 commits into from Jan 28, 2020
Merged

Conversation

Eric-Arellano
Copy link
Contributor

This change is meant to give a nudge to users in the direction of the V2 test runner. By having them switch to --no-fast now, we lower the barrier to entry to the V2 test runner, which must run with --no-fast.

This is meant to avoid a Python 2 vs. Python 3 situation where the cost to migrate from the V1 test runner to the V2 implementation is too costly such that users stick with V1 and the Pants community now bifurcates.

@stuhood
Copy link
Sponsor Member

stuhood commented Jan 23, 2020

To explain the meaning behind my shipit above: Twitter will be ready for this.

@Eric-Arellano Eric-Arellano changed the title WIP: Default to --no-fast and deprecate --fast option WIP: Deprecate --fast option Jan 23, 2020
@@ -848,7 +843,6 @@ def test_coverage_issue_5314_all_source_roots(self):
self.assertEqual([1, 2, 5, 6], all_statements)
self.assertEqual([2], not_run_statements)

@ensure_cached(PytestRun, expected_num_artifacts=1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this because @ensure_cached is hardcoded to check that there is only one cache_subdir. With sharding, we have two subdirs. Rewriting TaskTestBase to allow for multiple cache_subdirs would be a lot of work for this legacy code.

def test_coverage_auto_option_mixed_multiple_targets(self):
all_statements, not_run_statements = self.run_coverage_auto(targets=[self.green, self.red],
failed_targets=[self.red])
self.assertEqual([1, 2, 5, 6], all_statements)
self.assertEqual([], not_run_statements)
self.assertEqual([2], not_run_statements)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure this one out.

This is now the same output as test_coverage_auto_option_red. I suspect what's happening is that self.green says every statement ran, but self.red says statement 2 did not, so the combined result says statement 2 did not run?

cc @jsirois, do you recall if V1 Pytest --no-fast will attempt to merge coverage results? I know that's a tricky task @TansyArron has spent much time on for V2.

@Eric-Arellano Eric-Arellano changed the title WIP: Deprecate --fast option Deprecate --fast option Jan 28, 2020
@Eric-Arellano
Copy link
Contributor Author

Finally got this green! Ready for review.

Copy link
Sponsor 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!

@@ -184,10 +183,7 @@ def test_unit_tests_with_different_sets(self):
])
self.assert_success(run)

@expectedFailure
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

🎸

@Eric-Arellano Eric-Arellano merged commit b531b8a into pantsbuild:master Jan 28, 2020
@Eric-Arellano Eric-Arellano deleted the remove-fast branch January 28, 2020 23:20
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.

None yet

2 participants