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 cross-platfrom issues with test suite #529

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jnmclarty
Copy link

@jnmclarty jnmclarty commented Jun 2, 2016

Several minor issues related to test suite inhibit a smooth development workflow on Windows.

The commits are labelled appropriately with the changes. They are independent.

Notice the '*' logic is ignored on the windows test suite. That edge case (feature) may or may not work depending on minor release of python. This is an assumption based on upstream usage. Testing for it, is more of a hassle than it's likely worth.

This was tested locally against 2.7, 3.4 and 3.5. I'm counting on travis for the rest.

I have a more strategic version of the normalize function, which I may finish later and send after this is merged.

@jnmclarty
Copy link
Author

jnmclarty commented Jun 2, 2016

Ian, the risk of this PR is much lower than all the solution paths we discussed before you left. I erred on the side of fixing the test suite, without attempting cleverness, rather than implementing a strategic solution which would be a backwards incompatibility nightmare. Right now, the folders given by users, are allowed not to exist. All the common approaches validate the strings using existence, rather than say, a regex based approach.

split = line.split(':')
if 'win' in sys.platform:
if len(split) == 5:
drive, path, x, y, msg = split
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave comments explaining what kind of situation would result in having 5 items? (Likewise for the elif checking for a length of 4) Bonus points for an example in the comment.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, that will be helpful as we continue to build up the windows support and verify that use case.

@jnmclarty
Copy link
Author

Hi Folks, it appears your comments slipped through my inbox. I'll get to the feedback on the weekend.

@jnmclarty
Copy link
Author

Merge?

@jayvdb
Copy link
Member

jayvdb commented Jul 16, 2016

The changes to normalize_paths are a breaking change on Windows, and it looks like they will break https://github.com/jayvdb/flake8-putty , which converts os.sep to / because that is what pycodestyle handles correctly. (jayvdb/flake8-putty@a0c7c604615)

To avoid this being a breaking change, normalize_paths needs to handle os.sep and os.altsep on Windows.

Then your changes to testsuite/test_util.py are not needed.

@sigmavirus24
Copy link
Member

The changes to normalize_paths are a breaking change on Windows

Can you explain why they're a breaking change? Does it break plugins or something else?

@jayvdb
Copy link
Member

jayvdb commented Jul 16, 2016

No. Command line usage also breaks. The changes to the test suite show how it breaks.

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.

4 participants