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 a linter and reformatter for python infra and test code #8154

Open
thejohnjansen opened this issue Nov 10, 2017 · 11 comments
Open

Add a linter and reformatter for python infra and test code #8154

thejohnjansen opened this issue Nov 10, 2017 · 11 comments

Comments

@thejohnjansen
Copy link
Contributor

I'm doing line by line comparisons to try to figure out why Edge is not able to run the WebDriver tests and see subtle differences in formatting that make it difficult to see differences. @andreastt

@andreastt
Copy link
Member

I would expect wptrunner has some linting infrastructure already set up, and could piggyback on this.

@domenic
Copy link
Member

domenic commented Nov 12, 2017

A linter already exists, but it doesn't enforce formatting preferences on the test writer (which is IMO a good thing).

In the past when Edge has tried to contribute changes they have come along with a large variety of stylistic changes. See e.g. #6499. Perhaps the issue is that Edge has some reformatter that they apply, which they should be turning off when operating on a shared, collaborative codebase?

@foolip
Copy link
Member

foolip commented Nov 13, 2017

@thejohnjansen, can you elaborate on the original problem, what are the things going through "line by line comparison"?

Is the request that all of web-platform-tests have a consistent style to make it easier to read tests, that we have an optional reformatter to reduce the number of choices one has to make when writing tests, or something else?

@kereliuk
Copy link
Contributor

@thejohnjansen By line by line, are you talking about when you do wpt run and there is an error thrown but the output from WPT doesn't say where or give you enough of a stack trace to figure it out? (Even if its a syntax error, this happens)

@andreastt
Copy link
Member

To shed some more light on the matter, @thejohnjansen was complaining about stylistic differences in the Python wdspec tests (under /webdriver), and not generally for every other test type. I don’t think it makes sense to enforce a particular style for other test types.

It is true that there is a lot of stylistic inconsistency under /webdriver that a linter would help prevent. I think however that there are other, more compelling reasons to enable linting of Python tests:

  • Detect unnecessary, unused imports
  • Correct misuse of == for comparing True, False, and None
  • Detect other programming errors

I wouldn’t proclaim that I know a great deal about the best Python testing practices, but it seems we can add tox and flake8, which is used for wptrunner, quite easily without introducing any new dependencies. I have a large-ish patch locally that does this and fixes an array of small problems.

@foolip foolip changed the title The Web Platform Tests repo should have a linter for the tests and wptrunner Add a linter and reformatter for python infra and test code Nov 14, 2017
@foolip
Copy link
Member

foolip commented Nov 14, 2017

Renaming and adding some labels based on #8154 (comment). @thejohnjansen, if the issue is now narrower than you hoped for, please shout :)

@andreastt andreastt self-assigned this Nov 14, 2017
@thejohnjansen
Copy link
Contributor Author

Yes, the scope of this issue is accurate. I'm sorry for the initial confusion (and for not responding, I'm apparently not getting GitHub notifications). I was looking at a test file in the WebDriver directory and clicked to file a new issue, so I thought the issue would include the context of what I was looking at. It did not :-)

@foolip foolip removed the webdriver label Nov 2, 2018
@foolip
Copy link
Member

foolip commented Nov 2, 2018

We currently run flake8 as part of tox, together with pytest. I have found this a bit hard to understand. @jgraham, what would you think of having a dedicated test job for flake8, invoking it once?

@foolip
Copy link
Member

foolip commented Nov 2, 2018

I've sent #13865 to do that.

@thiagowfx
Copy link
Member

what would you think of having a dedicated test job for flake8, invoking it once?

#14893

@thiagowfx
Copy link
Member

Also, for a more recent discussion about the topic, see web-platform-tests/rfcs#171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants