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

Config Travis CI for two jobs #2463

Merged
merged 8 commits into from
Sep 23, 2020
Merged

Conversation

dhruvmanila
Copy link
Member

As discussed in #2439

Dividing Travis CI test into two jobs:

  • First job will be testing everything except for the project_euler directory
  • Second job will only run pytest on project_euler directory

NOTE: Before merging please tell me so that I can remove the branches section from .travis.yml file as that was done only for testing for which the results are here: https://travis-ci.org/github/dhruvmanila/Python/builds/729560689

Extra:
There's another error which I also wrote about in #2439 comment and I will paste it here:

The way mypy finds the python files are a bit tricky. In the Travis test, mypy isn't going inside the project_euler directory because there's no __init__.py file in it. We need to have the __init__.py file in every directory and subdirectory to tell mypy to look for python files in this directory as well which means mypy is skipping tons of files. It's checking only for 116 files while there are around 668 files with extension py with around 123 directories according to:

$ fd -t f -e py . | wc -l        # find files with extension py and count the lines
       668

$ fd -t d . | wc -l                 # find directories and count the lines
       123

I have written a bash script which will add all the init files in the directories where it's missing.
Do you want me to make another PR for it or just add it in here?

Describe your change:

  • Fix a bug or typo in an existing algorithm?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@dhruvmanila
Copy link
Member Author

Ignore the NOTE. I have removed the branches section.

One more thing about the mypy thing:
Out of 123 directories only 62 contains the __init__.py file and most of them are the project_euler directories.

Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Awesome!

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@cclauss
Copy link
Member

cclauss commented Sep 23, 2020

Do you want me to make another PR for [adding __init__.py files] or just add it in here?

That is a completely separate topic so please make that a separate PR.

Python is a brand name so please capitalize except when referring to the command itself: python3 -c print("Hello")

@cclauss
Copy link
Member

cclauss commented Sep 23, 2020

Can you please open an issue on https://github.com/python/mypy to verify the requirement for __init__.py files to get mypy to work. It seems to run against the current of not requiring those files to make packages and modules in modern version of Python. It would be a bummer to create a ton of files that we do not need.

@cclauss cclauss merged commit 6e6a49d into TheAlgorithms:master Sep 23, 2020
@dhruvmanila
Copy link
Member Author

Sorry for not putting Python in capitalize. I will keep that in mind. Thanks for pointing it out.

There's already an open issue about it: python/mypy#6385
Here's Guido van Rossum explaining what is going on: python/mypy#6385 (comment)

It seems not to be fixed and the only option right now is to add __init__.py files if we want mypy to check all the scripts.

@dhruvmanila dhruvmanila deleted the config-travis branch September 23, 2020 11:35
@cclauss
Copy link
Member

cclauss commented Sep 23, 2020

I put mypy into || true mode because of python/mypy#7907 so let's hold off on adding all the __init__.py files for a while.

@dhruvmanila dhruvmanila restored the config-travis branch September 23, 2020 11:42
@dhruvmanila dhruvmanila deleted the config-travis branch September 23, 2020 11:42
@dhruvmanila
Copy link
Member Author

Ok. Sorry for the multiple deletions, I wanted to clean the history properly on my local copy.

@dhruvmanila
Copy link
Member Author

Do you want me to fix the mypy errors without adding the __init__.py file?
I will add the __init__.py files and check for the errors and create PR only for the change through a different branch.
If so, should I create different PR for each file or group a few files together because the errors are tiny?

@cclauss
Copy link
Member

cclauss commented Sep 23, 2020

Let's hold off on mypy-related changes for now (unless you just want to fix the Project Euler ones).

Instead, let's focus on pytesting the return values of the Euler algorithms.

@dhruvmanila
Copy link
Member Author

Alright. So a few ideas right out of head are:

  • Make a Python script test_project_euler.py in project_euler directory which will collect all the files and execute them comparing the answers to the actual ones (probably a bad idea).
  • Make a test file in each folder testing for every solution files which seems more probable, but then what about new problems getting added? We could have a naming convention for the final function which will return the answer and compare that to the actual ones. I think the naming convention already exists for the files, but not mentioned in the README.md file.

@cclauss
Copy link
Member

cclauss commented Sep 23, 2020

Let's try to build #1 (single file) but cover just subdirectories problem_01 thru problem_09 with a single test file. We can see how difficult it is and optimize the code. If we create a test file per directory, it will be a ton of work to keep them in sync.

@dhruvmanila
Copy link
Member Author

Alright, I took a look at the files and found some inconsistencies:
(Should we do this conversation in a separate PR?)

  • Some folder names contain trailing zeroes and some don't like from problem_01 to problem_09 and then there are none. So we need to have a naming convention which either says to have trailing zeroes in all the new directories or it should have none. I would prefer the latter (No trailing zeroes: problem_1, problem_2, ...problem_99, problem_100...
  • Similar all solutions files should be like sol1.py, sol2.py, ... Everyone does follow this convention but I did see one file which is problem_27_sol1.py. No trailing zeroes in here as well: sol1.py, sol2.py, ...sol9.py, sol10.py...
  • Next is that all solution files should contain a solution function which takes no input or defaults to the question's input (which is preferred as that allows doctest for different inputs) and returns the answer. Right now, some functions take input and some don't, the ones who take doesn't default to the question's input and so we would have to manually enter it.

What do you think about the above-mentioned points? If we implement them, testing will be easier from one file otherwise we will have to manually look at each file to see how it returns the answer and check accordingly.

@cclauss
Copy link
Member

cclauss commented Sep 24, 2020

Please open a new issue for this effort.

Let’s not change the directory names...

for i in range(101):
    print(f"problem_{i:02}")

We should use Pathlib to find all Python files in each directory... As long as they end in .py and do not contain test_ then we should assume they are valid.

Let’s start by trying to import solution() from each file and call it with no parameters to see how far we get. We should use logging and write all errors to a log so we have a hitlist of files to investigate.

  1. Could not import solution()
  2. Could not call solution() with no parameters
  3. Calling solution() with no parameters gave the wrong answer

@dhruvmanila
Copy link
Member Author

Alright, I'll get on it.

stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
* Testing Travis CI configuration for project Euler

* Fix: Installing mypy and pytest-cov for testing

* Remove unnecessary checks for project_euler job

* Removing branches section

* Update .travis.yml

* Update .travis.yml

* Update .travis.yml

* Update .travis.yml

Co-authored-by: Christian Clauss <cclauss@me.com>
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.

2 participants