Skip to content

Conversation

atodorov
Copy link
Contributor

@aerostitch I've taken your changes from #256 and squashed related commits together and removed some things and updated the order of operations.

  • try to import test classes from pylint.testutils - will work once we have 2.5 released
  • for Travis CI just git clone the 2.4 branch from which we can import tests_functional. The code will try to use PYLINT_TEST_FUNCTIONAL_DIR but will default to the Travis location
  • if that fails try to use an older location

The only reason you may need these changes is if you are trying to either develop/test pylint-django locally or maybe you are trying to build a binary package for a Linux distro. The changes above should make it possible to workaround the current state of affairs.

Or alternatively install pylint from master branch and use that instead.

Tell me what you think ?

instead make use of PYLINT_TEST_FUNCTIONAL_DIR env variable.

This is only useful if you are trying to run the test suite and
build a new package! Apparently some distributions (Debian) add
back the tests/ directory into the package so they can import
test_functional.py from there!

In Travis CI - use the 2.4 branch for testing
@coveralls
Copy link

coveralls commented Nov 30, 2019

Pull Request Test Coverage Report for Build 922

  • 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 Coverage Status
Change from base Build 889: 0.0%
Covered Lines: 5160
Relevant Lines: 5824

💛 - Coveralls

@coveralls
Copy link

coveralls commented Nov 30, 2019

Pull Request Test Coverage Report for Build 922

  • 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 Coverage Status
Change from base Build 889: 0.0%
Covered Lines: 5805
Relevant Lines: 6552

💛 - Coveralls

@aerostitch
Copy link
Contributor

Hi @atodorov!
The PR doesn't fix the problem of incorrect tox configuration so the test against the django-master tox target is kinda useless as-is and any specific tests where we try to overwrite package versions, especially for pylint will be prone to issues due to the multi-versioning of dependencies installed halfway on the system level and halfway in the tox virtual environment. It would be good to address these issues sooner than later to avoid problems in the future (maybe in a separate PR?) IMHO.
But otherwise it looks good.
Thanks.
Joseph

@aerostitch
Copy link
Contributor

The reason I say django-master is useless is that it tests against pylint 2.4 (system-level package) instead of pylint master as configured in the tox env. You can see more details there: #256 (comment)

@atodorov
Copy link
Contributor Author

@aerostitch just so that we're on the same page. Was your original intention behind #256 to fix the ste suite running against django-master or something else?

@aerostitch
Copy link
Contributor

No it wasn't but you asked me to fix the test suite in that PR and that's where I found the tox issue which is part of fixing the test suite. Right now your tests pasd on that tox config only because you're testing against the wrong package version. #256 would have had a lot less commits if I'd had stick to the original intent.

@atodorov
Copy link
Contributor Author

atodorov commented Dec 2, 2019

No it wasn't but you asked me to fix the test suite in that PR and that's where I found the tox issue which is part of fixing the test suite.

@aerostitch right, I did ask you to fix an unrelated issue but the original PR became too big. As for testing against django-master+pylint-master I will need to experiment a bit first. However this is outside of the current scope.

Above you said this PR looks good. Does it fix your original problem that distros add back the test files to the package ? If yes, let me know if you prefer to merge #256 (minus the django-master changes) or if I should go ahead and merge this one?

@aerostitch
Copy link
Contributor

@atodorov the merge of either works for me. Thanks!

@atodorov atodorov merged commit 23040e3 into master Dec 2, 2019
@atodorov atodorov deleted the pr256 branch April 8, 2021 12:14
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.

3 participants