Do not rely on the version number to know if the tests are present and fixing test matrix #256

aerostitch · 2019-11-24T19:03:37Z

Hi guys,

Sometimes the distro re-add the files that were removed from the pip packages and in this case it will probably be the case in a few days, so maybe it would be better to check if the test directory exists instead of relying on the version. What do you think?

Thanks for your help,
Joseph

coveralls · 2019-11-24T19:08:17Z

Pull Request Test Coverage Report for Build 916

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 88.599%

Totals
Change from base Build 889:	0.0%
Covered Lines:	5160
Relevant Lines:	5824

💛 - Coveralls

coveralls · 2019-11-24T19:08:17Z

Pull Request Test Coverage Report for Build 892

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at ?%

Totals
Change from base Build 889:	0.0%
Covered Lines:
Relevant Lines:	0

💛 - Coveralls

aerostitch · 2019-11-24T19:09:37Z

Seems the tests have been failing for a bit now.

…nt variable

aerostitch · 2019-11-24T19:47:09Z

I also added the ability to overwrite the path of the pylint test folder using the PYLINT_TESTS_PATH environment variable. This way, if you don't want to have your pylint tests installed in $HOME/pylint/tests when you do local testing you can. Sounds reasonable?

atodorov · 2019-11-24T20:32:38Z

@aerostitch your patch looks good and TBH I was about to approve it and merge it.

However the reason tests are failing now is that the classes we consume from test_functional (inside pylint_django/tests/test_func.py) have been moved to pylint.testutils which your changes will not help to resolve.

I am +1 about the idea to be able to configure the additional import path but maybe you can make it a bit more flexible and try the import from 2 different places.

While typing this I realize that the last paragraph can be a simple import within try/except block below the current changes. What do you think? Care to make Travis happy again ?

carlio · 2019-11-24T20:44:00Z

The compat module is here for that very reason, the dependencies of pylint_django are very volatile so things like this crop up a lot, so if possible consider adding to:
https://github.com/PyCQA/pylint-django/blob/master/pylint_django/compat.py

carlio · 2019-11-24T20:44:40Z

I know it sucks and is ugly but you gotta do what you gotta do.

aerostitch · 2019-11-25T00:53:41Z

Thanks for the review guys.
@carlio I do understand the use of compat for the module but should we add the test deps in it even if the end user will probably not run the tests?

@atodorov I think we only use test_functional from pylint's test suite so I'll look into making the import in a try/catch to manage that and rename the environment variable to be more explicit.

…ta do it the other way

…nc with the installed pkg

… to serve the transition

… pylint 2.5

…int 2.4 when it should not

aerostitch · 2019-11-25T02:59:41Z

So I've been able to have the tests pass.
A few issues we had:

when running tests on pylint 2.4 we were also loading the git repo which was in 2.5.0-dev with a lot of changes, so that was problematic. To fix that I cloned the repo on the 2.4 branch.
when running tests on pylint 2.5 as @atodorov was saying the classes have been moved from test_functional to pylint.testutils, so added a few changes for that too.

Right now I'm facing an issue when it seems that even with pylint 2.5 installed it looks for pylint 2.4 code which is weird, so still looking into it but having trouble reproducing locally. Seems like something related to caching on travis or something like that.

aerostitch · 2019-11-25T03:10:09Z

So I placed the following line right after the import pylint:

print('DEBUG: pylint version: {}'.format(pylint.__version__))

And in the DJANGO=master use case when we are supposed to have pylint use the git clone of master on pylint based on the tox description, I get pylint installed as 2.5 (pylint==2.5.0.dev1) but still showing as 2.4.4 in the debug logs and I don't understand why:

py36-django-master create: /home/travis/build/PyCQA/pylint-django/.tox/py36-django-master
py36-django-master installdeps: Django, git+https://github.com/pycqa/astroid@master, git+https://github.com/pycqa/pylint@master
py36-django-master inst: /home/travis/build/PyCQA/pylint-django/.tox/.tmp/package/1/pylint-django-2.0.13.zip
py36-django-master installed: astroid==2.4.0,Django==2.2.7,isort==4.3.21,lazy-object-proxy==1.4.3,mccabe==0.6.1,pylint==2.5.0.dev1,pylint-django==2.0.13,pylint-plugin-utils==0.6,pytz==2019.3,six==1.13.0,sqlparse==0.3.0,toml==0.10.0,typed-ast==1.4.0,wrapt==1.11.2
py36-django-master run-test-pre: PYTHONHASHSEED='4038369408'
py36-django-master run-test: commands[0] | coverage run pylint_django/tests/test_func.py -v
DEBUG: pylint version: 2.4.4

See: https://travis-ci.org/PyCQA/pylint-django/jobs/616459641?utm_medium=notification

aerostitch · 2019-11-25T04:48:30Z

Alright, we have the tests installation setup properly now.
https://travis-ci.org/PyCQA/pylint-django/builds/616489961?utm_medium=notification&utm_source=github_status

Now we have some tests failing on the master branch of pylint, so I'll have a look at those now but at least we test with what we're expecting to be testing.

aerostitch · 2019-11-25T05:11:07Z

I wonder if the error we get wouldn't be due to an issue in pylint:

    def parse_expected_output(stream):
>       return [OutputLine.from_csv(row) for row in csv.reader(stream, "test")]
E       _csv.Error: unknown dialect

aerostitch · 2019-11-25T05:59:18Z

Seems it's not really that.
It seems that running the following command gets you a stacktrace (aka not a valid csv output) when using pylint on master:

$ PYTHONPATH=. pylint --load-plugins=pylint_django pylint_django/tests/input/func_noerror_foreign_key_package.py
Traceback (most recent call last):
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/__init__.py", line 91, in _inference_tip_cached 
    return iter(_cache[func, node])
KeyError: (<function infer_key_classes at 0x7fa5a4570cb0>, <Call l.10 at 0x7fa5a3a8f3d0>)                                                                                                                          
                                                                                                         
During handling of the above exception, another exception occurred:                                                                                                                                                
                                                    
Traceback (most recent call last):                                                                                                                                                                                 
  File "/home/joseph/.pyenv/bin/pylint", line 11, in <module>
    load_entry_point('pylint==2.5.0.dev1', 'console_scripts', 'pylint')()                                                                                                                                          
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/__init__.py", line 23, in run_pylint
    PylintRun(sys.argv[1:])                                                                              
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/lint.py", line 1771, in __init__
    linter.check(args)                                                                                   
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/lint.py", line 981, in check
    self._check_files(self.get_ast, self._iterate_file_descrs(files_or_modules))                    
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/lint.py", line 1010, in _check_files
    self._check_file(get_ast, check_astroid_module, name, filepath, modname)                     
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/lint.py", line 1036, in _check_file
    check_astroid_module(ast_node)                                                                                                                                                                                 
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/lint.py", line 1169, in check_astroid_module
    ast_node, walker, rawcheckers, tokencheckers                                                                                                                                                                   
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/lint.py", line 1213, in _check_astroid_module
    walker.walk(ast_node)                                                                                                                                                                                          
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/utils/ast_walker.py", line 77, in walk
    self.walk(child)                                                                                                                                                                                               
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/utils/ast_walker.py", line 74, in walk
    callback(astroid)                                                                                                                                                                                              
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 55, in augment_func
    augmentation(chain, node)                                                                            
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 146, in do_suppress
    chain()                                                                                                                                                                                                        
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 54, in chain
    old_method(node)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 55, in augment_func
    augmentation(chain, node)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 146, in do_suppress
    chain()
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 54, in chain
    old_method(node)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 55, in augment_func
    augmentation(chain, node)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 146, in do_suppress
    chain()
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint_plugin_utils/__init__.py", line 54, in chain
    old_method(node)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/checkers/classes.py", line 759, in visit_classdef
    self._check_bases_classes(node)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/checkers/classes.py", line 1558, in _check_bases_classes
    unimplemented_abstract_methods(node, is_abstract).items(),
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/checkers/utils.py", line 808, in unimplemented_abstract_methods
    inferred = safe_infer(obj)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/pylint/checkers/utils.py", line 1089, in safe_infer
    value = next(inferit)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/decorators.py", line 131, in raise_if_nothing_inferred
    yield next(generator)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/decorators.py", line 95, in wrapped
    res = next(generator)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/bases.py", line 133, in _infer_stmts
    for inferred in stmt.infer(context=context):
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/node_classes.py", line 353, in infer
    return self._explicit_inference(self, context, **kwargs)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/wrapt/wrappers.py", line 564, in __call__
    args, kwargs)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/__init__.py", line 93, in _inference_tip_cached
    result = func(*args, **kwargs)
  File "/home/joseph/git/aerostitch/pylint-django/pylint_django/transforms/foreignkey.py", line 96, in infer_key_classes
    MANAGER.ast_from_module_name(module_name)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/manager.py", line 184, in ast_from_module_name
    raise e
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/manager.py", line 135, in ast_from_module_name
    found_spec = self.file_from_module_name(modname, context_file)
  File "/home/joseph/.pyenv/lib/python3.7/site-packages/astroid/manager.py", line 230, in file_from_module_name
    raise value
astroid.exceptions.AstroidImportError: Failed to import module input.models with error:
No module named input.models.

aerostitch · 2019-11-25T06:20:51Z

Reviewing all the test cases that fail on travis with pylint 2.5 is basically all test cases that are not a noerror kind of test (aka all the tests that actually push an output are having that unknown dialect issue, the ones that don't have an output don't :) ).
So I guess something has changed in the way pylint handles the csv output which makes the parser not like what we send to it anymore.
I see that you guys seem pretty used to commit there. Would you have any idea on how to fix this?
I'm kinda running out of ideas.
Thanks for your help!
Joseph

atodorov · 2019-11-25T09:46:09Z

@aerostitch thanks for your efforts. This appears to be more messier that it should. I will look at it in the next few days b/c I need to understand what's going on here before doing anything else. I was secretly hoping the change will be smaller.

aerostitch · 2019-11-25T19:52:39Z

Thanks @atodorov

Trying to do a summary so that's easier:

1. Lots of commits - can be squashed or separated in another PR

There are a lot of commits mainly because I couldn't reproduce some of the issues locally and a this build was already breaking before my change so understand what was the origin of the breakages took a lot of tries.

I can squash the commits if it's easier to understand (I was thinking this full PR could be squashed) or I can create a separate PR to fix the issue I've seen with the build matrix (not talking about the tests, just the build matrix).

2. Problems currently fixed in the PR

The issues are divided in the following pools:

change the import to support pre-pylint 2.4, pylint 2.4 and pylint 2.5
change the clone command to retrieve the tests from the version 2.4 and not 2.5 (which is what you have on the master branch the clone ended up on)
don't install the dependencies on the system directly using pip install -e .[for_tests] as it installs the install_require and the for_test section of the extra_require which brings, in the case of the django-master target both pylint 2.4.4 and pylint 2.5 (and as pylint-plugin-utils installs pylint 2.4, that's the one which is chosen by python so you ended up testing only on pylint 2.4 everywhere). Just let tox do the installs in its own venv
add the missing dependencies in tox to be able to install everything without doing the problematic install step described above.

3. What's left to do? - Help needed on that

The current status is that the tests overall are running with the expected version of pylint and only the ones related to the pylint master branch fail.

Those tests fail due with an unknown dialect error in pylint's csv parser when an output is added by pylint_django apparently, so it's a total of 6 test cases. And I'm not sure if those failures are due to a bug in pylint or pylint_django not respecting a new norm for the output of the plugins in the tests.

Do not rely on the version number to know if the tests are present

e7a617d

Add the ability to overwrite the pylint tests path using an environme…

c03d78a

…nt variable

aerostitch added 12 commits November 24, 2019 17:14

test_functional has been moved to pylint.testutils in pylint 2.5

5126c5e

Fix typo

dbc50cb

testutils existed before they moved the functional tests to it so got…

cc174d0

…ta do it the other way

Trying the imports in another way

bffd5a3

Checking for AttributeError instead of ImportError

23718e9

Try to avoid loading different version of the package if possible

d131a0c

Handle different error types

46bd41e

Use ImportError as ModuleNotFoundError is not always available

1624261

Cloning the master sometimes leads to test_functional not being in sy…

cfe107c

…nc with the installed pkg

Make sure that the cloned dir is using pylint 2.4 as it is only there…

1bd7964

… to serve the transition

Adding a debug message to make sure we are taking the right path with…

0bff40c

… pylint 2.5

Add comments and some debug to understand why it is still loading pyl…

5f9c2bf

…int 2.4 when it should not

aerostitch force-pushed the pylint_tests branch from e862fc5 to 7f52a76 Compare November 25, 2019 03:29

Renaming the env to make sure there is no mismatch with the package name

da2146c

aerostitch force-pushed the pylint_tests branch from 7f52a76 to da2146c Compare November 25, 2019 03:30

aerostitch added 3 commits November 24, 2019 19:32

Do not pre-install the dependencies. Let tox take care of that.

8656e0a

add coverage as global dep

9694d2c

Add the missing defaults from for_test

dc00577

aerostitch added 4 commits November 24, 2019 19:56

Add more missing dependencies

1a12066

Missing dep again

703ed34

What is happening?

ff8e1e2

Django_tables2 depends on django

fba1a56

Remove debug messages

facd0b8

Fix the module full path when using foreign key in tests

1ef7453

aerostitch changed the title ~~Do not rely on the version number to know if the tests are present~~ Do not rely on the version number to know if the tests are present and fixing test matrix Nov 25, 2019

atodorov mentioned this pull request Nov 30, 2019

Another take on PR #256 so we can have tests working again #257

Merged

atodorov closed this Dec 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not rely on the version number to know if the tests are present and fixing test matrix #256

Do not rely on the version number to know if the tests are present and fixing test matrix #256

aerostitch commented Nov 24, 2019

coveralls commented Nov 24, 2019 •

edited

coveralls commented Nov 24, 2019

aerostitch commented Nov 24, 2019

aerostitch commented Nov 24, 2019 •

edited

atodorov commented Nov 24, 2019

carlio commented Nov 24, 2019

carlio commented Nov 24, 2019

aerostitch commented Nov 25, 2019

aerostitch commented Nov 25, 2019 •

edited

aerostitch commented Nov 25, 2019

aerostitch commented Nov 25, 2019

aerostitch commented Nov 25, 2019 •

edited

aerostitch commented Nov 25, 2019 •

edited

aerostitch commented Nov 25, 2019 •

edited

atodorov commented Nov 25, 2019

aerostitch commented Nov 25, 2019

Do not rely on the version number to know if the tests are present and fixing test matrix #256

Do not rely on the version number to know if the tests are present and fixing test matrix #256

Conversation

aerostitch commented Nov 24, 2019

coveralls commented Nov 24, 2019 • edited

Pull Request Test Coverage Report for Build 916

💛 - Coveralls

coveralls commented Nov 24, 2019

Pull Request Test Coverage Report for Build 892

💛 - Coveralls

aerostitch commented Nov 24, 2019

aerostitch commented Nov 24, 2019 • edited

atodorov commented Nov 24, 2019

carlio commented Nov 24, 2019

carlio commented Nov 24, 2019

aerostitch commented Nov 25, 2019

aerostitch commented Nov 25, 2019 • edited

aerostitch commented Nov 25, 2019

aerostitch commented Nov 25, 2019

aerostitch commented Nov 25, 2019 • edited

aerostitch commented Nov 25, 2019 • edited

aerostitch commented Nov 25, 2019 • edited

atodorov commented Nov 25, 2019

aerostitch commented Nov 25, 2019

coveralls commented Nov 24, 2019 •

edited

aerostitch commented Nov 24, 2019 •

edited

aerostitch commented Nov 25, 2019 •

edited

aerostitch commented Nov 25, 2019 •

edited

aerostitch commented Nov 25, 2019 •

edited

aerostitch commented Nov 25, 2019 •

edited