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

./mach test-tidy doesn't ignore files underneath ignored directories #12225

Closed
jdm opened this issue Jul 4, 2016 · 8 comments
Closed

./mach test-tidy doesn't ignore files underneath ignored directories #12225

jdm opened this issue Jul 4, 2016 · 8 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jul 4, 2016

There's a check in get_file_list in tidy.py for whether the given file is in a directory that is in the ignored directories list. However, this only catches things like a/b/c.py if a/b/c is in the list - if the list contains a/b, then it doesn't get ignored as it should.

To reproduce this problem, make a change to tests/wpt/harness/wptrunner/executors/executorservo.py, since tests/wpt/harness is in the list of ignored directories.

Code: python/tidy/servo_tidy/tidy.py
Test: ./mach test-tidy --self-test (add a test to test_file_list in python/tidy/servo_tidy_tests/test_tidy.py)

@highfive
Copy link

@highfive highfive commented Jul 4, 2016

1 similar comment
@highfive
Copy link

@highfive highfive commented Jul 4, 2016

@highfive
Copy link

@highfive highfive commented Jul 4, 2016

Please make a comment here if you intend to work on this issue. Thank you!

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jul 4, 2016

Damn. I swear I'd thought about this before 😑

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jul 4, 2016

I'll take it!

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jul 4, 2016

Go ahead! Feel free to ping us at the #servo channel in IRC or comment here in case you need any help :)

@jdm jdm added the C-assigned label Jul 4, 2016
@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jul 6, 2016

Should I submit the PR with two commits(one for the fix and another for the test) or is a single one just fine?

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jul 6, 2016

That's certainly up to you. We generally ask people to squash commits only if/when a future commit fixes the previous commit.

bors-servo added a commit that referenced this issue Jul 7, 2016
test-tidy command now ignores files in subdirectories of ignored dirs

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12225 .

I did not add a test for this, instead I added a directory with an empty file inside the ignored directory, and checked whether this file was being ignored or not.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12313)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 8, 2016
test-tidy command now ignores files in subdirectories of ignored dirs

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12225 .

I did not add a test for this, instead I added a directory with an empty file inside the ignored directory, and checked whether this file was being ignored or not.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12313)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.