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

test: use Travis CI to run tests on every pull request #1752

Closed
wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented May 14, 2019

Use flake8 to automate the discovery of Python syntax errors and undefined names.

This is a second attempt at #1336 which got into a bad git-state...

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

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 cclauss force-pushed the patch-2 branch 2 times, most recently from 40eceb4 to e2d38ab Compare May 14, 2019 17:53
@rvagg
Copy link
Member

rvagg commented Jun 7, 2019

I don't know how to hook this up and don't think I have permissions either. @nodejs/automation can you help?

@cclauss
Copy link
Contributor Author

cclauss commented Jun 7, 2019

Someone who has admin rights would need to go to https://travis-ci.org/nodejs/node-gyp and flip the repo switch on.

@rvagg
Copy link
Member

rvagg commented Jun 7, 2019

It's not quite that simple, or at least it never used to be, because of a policy about giving org privs to external services. There's hoops that need to be jumped through to get the github-bot to react to changes in this repo and trigger Travis runs (although that may have changed?).

@cclauss
Copy link
Contributor Author

cclauss commented Jun 7, 2019

Is there an internal test runner available? Jenkins or something?

@targos
Copy link
Member

targos commented Jun 7, 2019

I enabled it. We have the GitHub app so the url is https://travis-ci.com/nodejs/node-gyp

@targos
Copy link
Member

targos commented Jun 7, 2019

@rvagg it's simpler since some time with github app because we can give permissions to Travis on a per repository basis (it's in the organization's config)

@cclauss
Copy link
Contributor Author

cclauss commented Jun 7, 2019

Thanks much!! Build in progress... https://travis-ci.com/nodejs/node-gyp/pull_requests

@targos
Copy link
Member

targos commented Jun 7, 2019

Shouldn't it end up in a failed state instead of errored?

@cclauss
Copy link
Contributor Author

cclauss commented Jun 7, 2019

The tests now pass with one noqa TODO added (#1772).

I am unclear what is the difference between failed and errored but I do know that all these issues and more for both Python 2 and Python 3 were flattened in https://github.com/refack/GYP

@targos
Copy link
Member

targos commented Jun 7, 2019

I think that anything that fails outside of the "script" phase results in an error.
Is it necessary to run flake8 before npm install? Otherwise I would do:

  • pip install and npm install in the "install" phase
  • flake8 and npm test in the "script" phase

@cclauss
Copy link
Contributor Author

cclauss commented Jun 17, 2019

Bump on this one please.

@rvagg
Copy link
Member

rvagg commented Jun 18, 2019

👍 will merge this if you fix up the commits, make it 2 or 3 commits - one for travis, one or two for the python fixes as you see fit.

@rvagg
Copy link
Member

rvagg commented Jun 20, 2019

also, there's a "TODO" in there with noqa, is that something that needs to be resolved prior to landing this?

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
Copy link
Contributor Author

cclauss commented Jun 20, 2019

I am OK with leaving the TODO open as it has been working without issue to date. Sometimes that situation can be caused by implicit imports.

@@ -1182,7 +1182,7 @@ def LoadVariablesFromVariablesDict(variables, the_dict, the_dict_key):
if variable_name in variables:
# If the variable is already set, don't set it.
continue
if the_dict_key is 'variables' and variable_name in the_dict:
if the_dict_key == 'variables' and variable_name in the_dict:

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Identity is not the same thing as equality in Python.

Proof:

>>> the_dict_key = 'variable'
>>> the_dict_key += 's'
>>> the_dict_key == 'variables'
True
>>> the_dict_key is 'variables'
False
>>>

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
Copy link
Member

rvagg commented Jun 20, 2019

landed in 051b6ed

@rvagg rvagg closed this Jun 20, 2019
@cclauss cclauss deleted the patch-2 branch June 20, 2019 11:31
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>
@rvagg rvagg mentioned this pull request Jun 21, 2019
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.

None yet

4 participants