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

Use Travis CI to run tests on every pull request #1336

Closed
wants to merge 2 commits into from
Closed

Use Travis CI to run tests on every pull request #1336

wants to merge 2 commits into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 14, 2017

The owner of the this repo would need to go to https://travis-ci.org/profile and flip the repository switch on to enable free automated flake8 testing of each pull request.

Use flake8 to find Python syntax errors and undefined names. There are Python 3 syntax errors and many undefined names which may raise NameError at runtime. This PR runs flake8 runs in two passes: The first looks at critical issues in stop-the-build mode and the second looks at style violations in everything-is-a-warning mode.

Output

Python 2: https://travis-ci.com/nodejs/node/jobs/135827428#L505
Python 3: https://travis-ci.com/nodejs/node/jobs/135827419#L501

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

The owner of the this repo would need to go to https://travis-ci.org/profile and flip the repository switch __on__ to enable free automated flake8 testing of each pull request.

Use flake8 to find Python syntax errors and undefined names.  There are Python 3 syntax errors and many undefined names which may raise NameError at runtime. Flake8 runs in two passes: The first looks at critical issues in stop-the-build mode and the second looks at style violations in everything-is-a-warning mode.
@cclauss cclauss mentioned this pull request Nov 14, 2017
@cclauss
Copy link
Contributor Author

cclauss commented Jul 22, 2018

@nodejs/gyp @bnoordhuis @maclover7 @richardlau @gibfahn @Fishrock123 @vsemozhetbyt @refack

Can we please get Travis CI tuned on for this project like other Node projects? There are existing tests and linting for JavaScript (#1497) and Python (this PR) that should be run on all pull requests before they are reviewed.

@richardlau
Copy link
Member

This is a policy decision for @nodejs/node-gyp to take (I'm not a comitter on this project). There is an existing https://ci.nodejs.org/job/nodegyp-test-pull-request on Node.js' own CI, but I think it currently only runs tests and not linting.

@gibfahn
Copy link
Member

gibfahn commented Jul 23, 2018

but I think it currently only runs tests and not linting.

That could be easily fixed by having the flake8 command be part of an npm run lint script that is run by the test command (so npm run test actually calls npm run lint && npm run tap).

I'm not a fan of this as it stands, as people will think that the test suite has been run successfully, when in fact only the linter has run. It's also a step that has to be run manually by contributors.

Can we please get Travis CI tuned on for this project like other Node projects?

@cclauss what exactly is your goal here? If it's to have flake8 run on test suites, then my suggestion above is the quickest way to do that.

If it's to enable travis for node-gyp, then that is a different discussion.

There are existing tests and linting for JavaScript (#1497) and Python (this PR) that should be run on all pull requests before they are reviewed.

Why should they be run on all pull requests before they are reviewed? As long as they are run before the PR is landed, a linter change is unlikely to be the difference between landing and closing a PR.

@gibfahn gibfahn mentioned this pull request Jul 23, 2018
2 tasks
@cclauss
Copy link
Contributor Author

cclauss commented Jul 23, 2018

The entire trend of CI/CD is about using automation to test early and test often. It questions the value of having humans review code before it has gone through some level of automated testing.

As example, let’s consider #1333 and #1334 where our code could be raising NameError exceptions at runtime. These PRs have been open for eight months with no comment. If flake8 was turned on and automated reports were seen by everybody who were trying to improve the code then many of them would put a 👍 on these PRs and they would have been merged months ago.

Sometime over the next 500 days this repo either needs to be ported to Python 3 or archived. Publicly visible automated testing is recommended for efficiently doing the former.

This is PR is not attempting to be the end solution. It is merely trying to get Travis CI turned on for this repo (as it is for nodejs/node, etc.) so that we can collaborate on creating better and better tests.

cclauss pushed a commit to cclauss/node that referenced this pull request Jul 23, 2018
__DO NOT MERGE__: This will run the [flake8](http://flake8.pycqa.org) linter on several Nodejs repos that contain Python code to detect syntax errors and undefined names which can raise NameError at runtime.  There is a discussion in nodejs/node-gyp#1336 about the importance of using tools like Travis CI to automate the discovery of code quality issues.

__E901,E999,F821,F822,F823__ are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc.  Most other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.  This PR therefore demonstrates such a flake8 run these Nodejs codebases.
* F821: undefined name `name`
* F822: undefined name `name` in `__all__`
* F823: local variable `name` referenced before assignment
* E901: SyntaxError or IndentationError
* E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree
@cclauss
Copy link
Contributor Author

cclauss commented Jul 23, 2018

@gibfahn
Copy link
Member

gibfahn commented Jul 23, 2018

This is PR is not attempting to be the end solution. It is merely trying to get Travis CI turned on for this repo (as it is for nodejs/node, etc.) so that we can collaborate on creating better and better tests.

Thanks for the reply, but that doesn't really answer my question.

If you want to add Travis testing to node-gyp, then Travis should be running npm test.

If you want to add flake8 to the test suite, it should be run as part of npm test, which should probably be a separate PR.

@cclauss
Copy link
Contributor Author

cclauss commented Oct 8, 2018

Is there someone who could re-implement this PR in a way that satisfies @gibfahn's request above?

I do not understand the npm approach well enough but this PR has been open for almost a year and I believe that we would all benefit from having some basic Python linting in place.

@cclauss
Copy link
Contributor Author

cclauss commented Oct 21, 2018

@gibfahn npm install ; npm test in place. @refack

@cclauss
Copy link
Contributor Author

cclauss commented Dec 4, 2018

@gibfahn Can you or someone else please mentor me through this issue? There are 42 open PRs and none of them go through any CI testing. No new commit in the past week. I opened this PR and #1337 over a year ago and one year from now Python 2 is end of life. Is it really so difficult to turn on Travis CI or any similar CI tool so that we can try to achieve Python 3 compatibility in time?

@mhdawson
Copy link
Member

I'm thinking the current version looks reasonable to me.

I don't think we necessarily want to integrate flake8 into the npm test step as that would force everybody who wants to run npm test to install that through pip. If we agree that makes sense we can always do it in a follow on PR.

In terms of running this on Travis before the review, we do that for Node core now, running the linter and a linux test when the PR is submitted so I think this aligns with that approach.

Given that we want to make progress on nodejs/TSC#642, it seems important to me to start getting info on whether changes will break when node-gyp is used with Python3.

@refack @thefourtheye what do you think?

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM assuming the node-gyp contributors agree as well.

@thefourtheye
Copy link
Contributor

I don't think we necessarily want to integrate flake8 into the npm test step as that would force everybody who wants to run npm test to install that through pip. If we agree that makes sense we can always do it in a follow on PR.

This makes sense to me and I am okay with the current approach as it is.


@gibfahn What do you think?

@cclauss
Copy link
Contributor Author

cclauss commented Feb 18, 2019

@gustavosizilio See #1336 (comment) for a URL to the Jenkins tests.

Copy link

@Flyingmana Flyingmana left a comment

Choose a reason for hiding this comment

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

looks reasonable

@Flyingmana
Copy link

Are there any strong arguments against activating TravisCI?
I dont think adding this and activating needs a policy decision or similar, as its only a supporting part. It does not replace or change any of the existing processes the project has.
It also does not break anything.
And having this merged may lead to further improvements of the travis setup and reduce communication roundtrips where a maintainer is needed.

Is there anything I did miss?

//cc @refack @maclover7

@cclauss cclauss closed this May 14, 2019
cclauss added a commit to cclauss/node-gyp that referenced this pull request May 14, 2019
This is a second attempt at nodejs#1336 which got into a bad git-state...

Use flake8 to find Python syntax errors and undefined names.  There are Python 3 syntax errors and many undefined names which may raise NameError at runtime. This PR runs flake8 runs in two passes: The first looks at critical issues in stop-the-build mode and the second looks at style violations in everything-is-a-warning mode.
cclauss added a commit to cclauss/node-gyp that referenced this pull request May 14, 2019
This is a second attempt at nodejs#1336 which got into a bad git-state...

Use flake8 to find Python syntax errors and undefined names.  There are Python 3 syntax errors and many undefined names which may raise NameError at runtime. This PR runs flake8 runs in two passes: The first looks at critical issues in stop-the-build mode and the second looks at style violations in everything-is-a-warning mode.
cclauss added a commit to cclauss/node-gyp that referenced this pull request May 14, 2019
This is a second attempt at nodejs#1336 which got into a bad git-state...

Use flake8 to find Python syntax errors and undefined names.  There are Python 3 syntax errors and many undefined names which may raise NameError at runtime. This PR runs flake8 runs in two passes: The first looks at critical issues in stop-the-build mode and the second looks at style violations in everything-is-a-warning mode.
cclauss added a commit to cclauss/node-gyp that referenced this pull request Jun 20, 2019
This is a second attempt at nodejs#1336 which got into a bad git-state...

Use flake8 to find Python syntax errors and undefined names.  There are Python 3 syntax errors and many undefined names which may raise NameError at runtime. This PR runs flake8 runs in two passes: The first looks at critical issues in stop-the-build mode and the second looks at style violations in everything-is-a-warning mode.
rvagg pushed a commit that referenced this pull request Jun 20, 2019
This is a second attempt at #1336 which got into a bad git-state...

Use flake8 to find Python syntax errors and undefined names.  There are Python 3 syntax errors and many undefined names which may raise NameError at runtime. This PR runs flake8 runs in two passes: The first looks at critical issues in stop-the-build mode and the second looks at style violations in everything-is-a-warning mode.

PR-URL: #1752
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Jun 21, 2019
This is a second attempt at #1336 which got into a bad git-state...

Use flake8 to find Python syntax errors and undefined names.  There are Python 3 syntax errors and many undefined names which may raise NameError at runtime. This PR runs flake8 runs in two passes: The first looks at critical issues in stop-the-build mode and the second looks at style violations in everything-is-a-warning mode.

PR-URL: #1752
Reviewed-By: Rod Vagg <rod@vagg.org>
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.

6 participants