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

add find_fixme task #6173

Merged
merged 2 commits into from
Dec 10, 2014
Merged

add find_fixme task #6173

merged 2 commits into from
Dec 10, 2014

Conversation

clytwynec
Copy link
Contributor

With the changes from https://github.com/edx/edx-platform/pull/6122, we will no longer be finding fixmes with run_quality or run_pylint. This change adds a way of finding only fixmes. I've added it to the all-tests.sh script in the quality shard, so that jenkins builds will find the fixmes (writing them to their own log) without failing the builds.

)

sh(
"{pythonpath_prefix} pylint --disable R,C,W,E --enable=W0511 "
Copy link
Contributor

Choose a reason for hiding this comment

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

should the enable switch be 'fixme' instead of W0511?

@benpatterson
Copy link
Contributor

What can we do to add some testing on this change?

@@ -69,9 +69,9 @@ def test_failure_on_diffquality_pep8(self):

# Underlying sh call must fail when it is running the pep8 diff-quality task
self._mock_paver_sh.side_effect = CustomShMock().fail_on_pep8
with self.assertRaises(SystemExit):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benpatterson I've added a test, but for some reason it seems to be causing these two other tests to fail locally (maybe on jenkins too). This change fixes them... but I'm not sure why my test causes the failure to begin with. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, try pulling it into a separate file. That might clean it up.

@clytwynec clytwynec force-pushed the clytwynec/add_find_fixme_method branch 2 times, most recently from a33361c to 175a066 Compare December 9, 2014 17:02
@clytwynec
Copy link
Contributor Author

@benpatterson Looks like moving the test to another file didn't actually fix it. :(

@clytwynec clytwynec force-pushed the clytwynec/add_find_fixme_method branch from 175a066 to 3ea27a3 Compare December 10, 2014 15:39
@clytwynec clytwynec force-pushed the clytwynec/add_find_fixme_method branch from 3ea27a3 to 7cb2665 Compare December 10, 2014 15:43
@clytwynec
Copy link
Contributor Author

@benpatterson I've removed the tests and addressed other comments.

@benpatterson
Copy link
Contributor

👍

clytwynec pushed a commit that referenced this pull request Dec 10, 2014
@clytwynec clytwynec merged commit 5007e98 into master Dec 10, 2014
@clytwynec clytwynec deleted the clytwynec/add_find_fixme_method branch December 10, 2014 16:08
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