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

run_all.py new special-comment mechanism & Urllib3Using.py #314

Merged
merged 10 commits into from Apr 4, 2019

Conversation

@tommyli3318
Copy link
Member

commented Mar 28, 2019

What does this PR do?

This PR changes the mechanism in which run_all checks for whether import requirements are satisfied. Now each test file should include a special comment in one of the following formats:

# nuitka-skip-unless-expression: expression to be evaluated
(a test would be skipped if this expression evaluated to false)
OR
# nuitka-skip-unless-imports: module1,module2,...
(a test would be skipped if any modules are missing)

The new helper function checkRequirements inside run_all.py will search for these comments in every test module and do the respective checks as specified.
This new mechanism makes it more clear what imports are required for each test file, as they will be stated in a special comment inside each test file. It also improves code readability and eliminates repetition in run_all.py

This PR also changed every existing test file to include the special comment stating their required imports

In addition, the new Urllib3Using.py module is added, which run_all.py uses to test Nuitka's compatibility with urllib3.

Why was it initiated? Any relevant Issues?

This is part of my Google Summer of Code proposal.

PR Checklist

  • Correct base branch selected? develop for new features and bug fixes too. If it's
    part of a hotfix, it will be moved to master during it.
  • All tests still pass. Check the developer manual about Running the Tests. There
    are Travis tests that cover the most important things however, and you are
    welcome to rely on those, but they might not cover enough.
  • Ideally new features or fixed regressions ought to be covered via new tests.
  • Ideally new or changed features have documentation updates.

@tommyli3318 tommyli3318 changed the title created Urllib3Using.py [gsoc] created Urllib3Using.py Mar 28, 2019

tommyli3318 added 2 commits Mar 29, 2019

@tommyli3318 tommyli3318 changed the title [gsoc] created Urllib3Using.py [gsoc] run_all.py new mechanism for checking required imports Mar 30, 2019

@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

@kayhayen Please check over the new special-comment mechanism I've implemented for run_all.py!

Also, Urllib3Using.py works fine using manual testing, but during automated testing, the following error is raised:

Comparing output of 'Urllib3Using.py' using 'C:\Users\Tommy\AppData\Local\Programs\Python\Python37-32\python.exe' with flags silent, expect_success, standalone, remove_output ...
--- C:\Users\Tommy\AppData\Local\Programs\Python\Python37-32\python.exe (stderr)
+++ nuitka (stderr)

@@ -1 +1,2 @@

+Nuitka:WARNING:Unresolved 'import' call at 'C:\Users\Tommy\AppData\Local\Programs\Python\Python37-32\lib\site-packages\urllib3\packages\six.py:82' may require use of '--include-plugin-directory' or '--include-plugin-files'.

Error, outputs differed.
Skipped, Win32ComUsing.py (win32com not installed for this Python version, but test needs it).

I understand that urllib3 should be fully working and this is just a warning message for the six compatibility layer, so should I just suppress errors like these?

@kayhayen

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

As for the __import__ warning consider this function:

def isWhiteListedImport(node):

I would say that six should be added here. Probably there is a bunch more of those, although I sort of feel that we should have to look and make sure that the line is not disguising hidden dependencies, but I am pretty confident I would know by now from user bug reports, so it feels unlikely.

@kayhayen
Copy link
Member

left a comment

This is good stuff, however needs a few more changes.

Generally, I would suggest you allow Python code behind the marker, and then execute it in Python, seeing if it crashes (ImportError), so do like hasModule does, but use sys.exit(__import__("flask"), and the user would specifiy # nuitka-skip-test-unless: import("flask").

Then it's not limited to modules, can be any expression, and of course can do checks in sys.version etc. as we see fit, and you would probably statically add import os, sys before the call. Let me know if I was clear enough.

Essentially that would move all the code of that kind into the test itself, and it would allow other things as well, e.g. checking if we could use a port.

tests/standalone/run_all.py Outdated Show resolved Hide resolved
tests/standalone/run_all.py Outdated Show resolved Hide resolved
tests/standalone/run_all.py Outdated Show resolved Hide resolved
tests/standalone/run_all.py Outdated Show resolved Hide resolved
@kayhayen
Copy link
Member

left a comment

Using the network is a bit special, otherwise this looks good though.

tests/standalone/Urllib3Using.py Outdated Show resolved Hide resolved

@kayhayen kayhayen added the gsoc2019 label Mar 30, 2019

@kayhayen kayhayen self-assigned this Mar 30, 2019

@tommyli3318 tommyli3318 reopened this Mar 31, 2019

@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

As for the __import__ warning consider this function:

Nuitka/nuitka/importing/Importing.py

Line 154 in 69183f6

def isWhiteListedImport(node):
I would say that six should be added here. Probably there is a bunch more of those, although I sort of feel that we should have to look and make sure that the line is not disguising hidden dependencies, but I am pretty confident I would know by now from user bug reports, so it feels unlikely.

Do you want me to whitelist it? Or should we leave it for now? And you mean to add six as a special case check inside isWhiteListedImport right? (e.g. all standard libraries and six would be whitelisted)

@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

Generally, I would suggest you allow Python code behind the marker, and then execute it in Python, seeing if it crashes (ImportError), so do like hasModule does, but use sys.exit(__import__("flask"), and the user would specifiy # nuitka-skip-test-unless: import("flask").

I understand what you mean, but need a little more elaboration. So shuold I make findRequiredModules so that it uses the approach you named above to check whether all the required imports exist, and return a boolean value, and skip the usage of hasModule altogether? And it would probably be renamed to hasRequiredImports, and it would just be called for every single test file. This would eliminate the need for using hasModule.

Then it's not limited to modules, can be any expression, and of course can do checks in sys.version etc. as we see fit, and you would probably statically add import os, sys before the call. Let me know if I was clear enough.

So how would this work in the case of Tkinter, where it's "Tkinter" for python2 and "tkinter" for python3? Also would we need to change the syntax for our special comment? Refer to my reply above:

I understand, but the issue with TkInter was that the import which should be checked for depends on python version, "Tkinter" for python2 and "tkinter" for python3. In this case without the "specials", a test might be skipped altogether even if the requirement was met, and changing elif to if wouldn't help the issue. If we do this without specials, should we develop a new syntax for the special comment? e.g. # nuitka-skip-unless: Tkinter || tkinter, or something along the lines of that?

@kayhayen

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

the issue. If we do this without specials, should we develop a new syntax for the special comment? > e.g. # nuitka-skip-unless: Tkinter || tkinter, or something along the lines of that?

I would be fine with this:

# nuitka-skip-unless: __import__("Tkinter" if sys.version_info[0] < 3 else "tkinter")

That means, the comment becomes code to evaluate. And it's run much like the import check currently is done with the "-c", and would have to be an expression, so you can feed it into a template of say import sys, os; sys.exit(%s) which we can use to tell if it passes. An exception (ImportError here) or a false expression can then both be detected from the exit code not being 0.

@kayhayen

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Then we can throw away the imports checking, and only use this, and have __import__("flask") and the like as the simple solution. The test runner would then only concern itself with that kind of thing, or you have both, # nuitka-skip-unless-expression and # nuitka-skip-unless-imports, your pick.

@tommyli3318 tommyli3318 changed the title [gsoc] run_all.py new mechanism for checking required imports run_all.py new mechanism for checking required imports Mar 31, 2019

@tommyli3318 tommyli3318 changed the title run_all.py new mechanism for checking required imports run_all.py new special-comment mechanism for checking required imports Mar 31, 2019

tommyli3318 added 3 commits Mar 31, 2019
@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

or you have both, # nuitka-skip-unless-expression and # nuitka-skip-unless-imports, your pick.

Updated with the new checkRequirements method supporting both, please check over.
University resumes tomorrow for me, so I'll probably be less active until the weekends.

@tommyli3318

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Updated Urllib3Using.py to use only on localhost, eliminating external source dependency. For SSL tests, should it be in a separate module?

@kayhayen

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Fantastic job, Tommy, I will merge once I put out the stable release, so this doesn't bring up unexpected failures. Most probably later today.

@kayhayen

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

As for SSL, I would use the same test. Using 2 tests will incur too much cost in compile times.

@tommyli3318 tommyli3318 changed the title run_all.py new special-comment mechanism for checking required imports run_all.py new special-comment mechanism & Urllib3Using.py Apr 3, 2019

tests/standalone/run_all.py Outdated Show resolved Hide resolved

@kayhayen kayhayen merged commit cb1e194 into Nuitka:develop Apr 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kayhayen

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I am very excited about this one. Really liking how this simplifies the Top 50 task by allowing to not have to edit the run_all.py for every package.

Thanks for your contribution!

@tab1tha

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@tommyli3318 this has helped me a lot. Thanks for your contribution!

@tab1tha tab1tha referenced this pull request Apr 4, 2019
3 of 3 tasks complete
kayhayen added a commit that referenced this pull request Apr 4, 2019
Tests: New special-comment mechanism and standalone test for urllib3 (#…
…314)

* Can now specifiy vital imports that decide if a test makes sense in a comment.

* Can now specify expressions that must work for a test to make sense in a comment

* Added Urllib3Using.py standalone test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.