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

Tests: Add search mode "All" #378

Merged
merged 6 commits into from Jun 13, 2019

Conversation

@bksahu
Copy link
Member

commented May 2, 2019

Fixes #372

Thank your for contributing to Nuitka!

!! Please check that you select the develop branch (details see below link) !!

Before submitting a PR, please review the guidelines:
https://github.com/kayhayen/Nuitka/blob/master/CONTRIBUTING.md

What does this PR do?

Why was it initiated? Any relevant Issues?

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.
"%s: Error, print of non-constant '%s'." % (
getSourceRef(statement),
getKind(print_arg)
if isinstance(search_mode, SearchModeAll):

This comment has been minimized.

Copy link
@kayhayen

kayhayen May 13, 2019

Member

One style complaint here. I think using isinstance is nearly always too involved with classes and their hierarchy. Instead, most often, a isXYZ or shallABC method that is asked for a feature or desired behavior better.

Also, in this concrete example, I dislike the sys.exit() being conditional. I think that is precisely what we want the search mode to decide.

Instead call search_mode.exit(filename, message =...), the default implementation will then call sys.exit(message) whereas the all mode can pick it up, and add it to the list of errors occurred.

nuitka/tools/testing/SearchModes.py Show resolved Hide resolved
@@ -341,8 +343,11 @@ def compareWithCPython(dirname, filename, extra_flags, search_mode, needs_2to3):
_removeCPythonTestSuiteDir()

if result != 0 and result != 2 and search_mode.abortOnFinding(dirname, filename):
my_print("Error exit!", result)
sys.exit(result)
if search_mode.isSearchModeAll():

This comment has been minimized.

Copy link
@kayhayen

kayhayen May 14, 2019

Member

I would rather make this search_mode.onErrorDetected(message) and not use a test method.

When I said that test methods of the isXY kind are better, that was true. But even better, make the search mode decide what's going to happen entirely, and just inform it about facts.

@bksahu

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@kayhayen I have found a weird bug. During last commit c01d046 somehow all the files I have changed has no executable permission due to which I am getting permission denied error while trying to run the tests. BTW before committing, I manually ran autoformat-nuitka-source and check-nuitka-with-pylint3.

@bksahu bksahu force-pushed the bksahu:all_search_mode branch from c01d046 to 4122432 May 16, 2019

# else:
# message = "Error exit!" + str(result)
# search_mode.exit(message)
message = "Error exit!" + str(result)

This comment has been minimized.

Copy link
@kayhayen

kayhayen May 16, 2019

Member

Why not use the "%" formatting?

This comment has been minimized.

Copy link
@bksahu

bksahu May 16, 2019

Author Member

Okay

@bksahu

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

@kayhayen So, should I close this PR?

@kayhayen

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

I was waiting for you to address my review comments, then merge it. The merge conflicts are probably from black formatting which I try to now apply to test runners as well. However, there can be an # isort:start trick missing in some,beware.

@bksahu

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Okay. I think I have already addressed all your reviews but I will look into it again today. Also I am unable to resolve the merge conflict as it says "Only those with write access to this repository can merge pull requests." Okay, I will try to rebase.

@bksahu bksahu force-pushed the bksahu:all_search_mode branch from d9edd46 to e3541a1 Jun 8, 2019

@bksahu

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

@kayhayen I'm ready for a review.

@@ -347,8 +349,12 @@ def compareWithCPython(dirname, filename, extra_flags, search_mode, needs_2to3):
_removeCPythonTestSuiteDir()

if result != 0 and result != 2 and search_mode.abortOnFinding(dirname, filename):
my_print("Error exit!", result)
sys.exit(result)
# if search_mode.isSearchModeAll():

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 10, 2019

Member

You probably meant to remove the commented out codes?

@@ -372,8 +378,11 @@ def checkCompilesNotWithCPython(dirname, filename, search_mode):
result = 2

if result != 1 and result != 2 and search_mode.abortOnFinding(dirname, filename):
my_print("Error exit!", result)
sys.exit(result)
if isinstance(search_mode, SearchModeAll):

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 10, 2019

Member

Not using onErrorDetected here, but probably should too.

def isCoverage(self):
# Virtual method, pylint: disable=no-self-use
return False

def onErrorDetected(self, message):
if self.isSearchModeAll():

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 10, 2019

Member

Please use overloading of methods. This should simply be the sys.exit I figure, while the method in the all search mode would do what it does. You do not need isSearchModeAll in this case, which was the point of using a method.

tests/optimizations/run_all.py Show resolved Hide resolved

def main():
search_mode = createSearchMode()

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 10, 2019

Member

This one is not using search modes at all yet, wrong to add it without a scan.

@bksahu

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

@kayhayen Should we have a testing module to test the "search modes"?

@kayhayen

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

No, I don't think that is needed. Us looking at them running will have to do. :)

.gitignore Show resolved Hide resolved
os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..")
),
)

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 11, 2019

Member

I noticed you did add the isort:start correctly for one runner, but basically all that to that sys.path insertion need to do it, sorry, that is a bunch.

@kayhayen

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Looks good aside those missing # isort:start markers that allow the isort to handle the sys.path extension to allow finding local nuitka from checkout gracefully. And of course, there is also still the method isSearchModeAll or so, which you stopped using, but has no right to exist.

Thanks for the good work. I am almost tempted to repair the isort:start myself.

@bksahu

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

@@ -22,6 +22,8 @@
import os
import sys

from nuitka.tools.testing.SearchModes import SearchModeAll

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 13, 2019

Member

Here is is actually harmful, and as unused ought to have been removed. The pylint doesn't cover these files yet. I am going to fix this myself though, no big deal,as Travis doesn't run these. Maybe it ought to though, I will explore that. And also adding the test runners to pylint coverage.

@kayhayen kayhayen merged commit 42f63ce into Nuitka:develop Jun 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
kayhayen added a commit that referenced this pull request Jun 13, 2019
Tests: Add search mode "All" (#378)
* Added search mode All

* Added "onErrorDetected()" method to let search mode decide

* Fixes for missing "# isort:start" and autoformat doing runners
kayhayen added a commit that referenced this pull request Jun 13, 2019
Tests: Add search mode "All" (#378)
* Added search mode All

* Added "onErrorDetected()" method to let search mode decide

* Fixes for missing "# isort:start" and autoformat doing runners
kayhayen added a commit that referenced this pull request Jul 8, 2019
Tests: Add search mode "All" (#378)
* Added search mode All

* Added "onErrorDetected()" method to let search mode decide

* Fixes for missing "# isort:start" and autoformat doing runners
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.