Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upTravis CI: Remove redundant tests #2523
Conversation
|
Thanks |
| - pytest --tb=no --no-summary --capture=no project_euler/validate_solutions.py || true # fail fast on wrong solution | ||
| - pytest --doctest-modules --durations=10 --cov-report=term-missing:skip-covered --cov=project_euler/ project_euler/ |
dhruvmanila
Sep 30, 2020
Member
By keeping the testing of Project Euler solutions in script section, Travis won't exit if it fails. If the test is in before_script section, Travis will exit.
cclauss
Sep 30, 2020
Author
Member
Why do we run that project_euler/validate_solutions.py thru pytest? It seems like a lot of complexity and overhead.
dhruvmanila
Sep 30, 2020
Member
I did do it without pytest but you mentioned (#2471 (comment)) to include it in as that is already included.
Currently, the algorithm is set in such a way to utilize the pytest.mark.parameterize decorator which calls the function with the arguments as problem_number and expected and at the end uses pytest.fixture to print all the error messages. This can be done without using pytest.
dhruvmanila
Sep 30, 2020
Member
It would be similar to this without pytest: https://github.com/TheAlgorithms/Python/blob/f1d60aca9e8c6cba013e5e5571d180f062bb3452/project_euler/validate_solutions.py
The script will be self-contained and will need no dependencies.
cclauss
Sep 30, 2020
Author
Member
It was run automatically by pytest when its name was project_euler/solution_test.py but that stopped being the case when the filename was changed to something that does not obey pytest discovery rules. Let's not mess with it for now because we need to run in || true mode.
dhruvmanila
Sep 30, 2020
•
Member
Yes, but you mentioned something about failing fast so I changed the filename and used another run for that file and that's why I commented here. If you put the command in script, it's not going to stop the build if it fails. Those options are to suppress all the output from pytest and print our custom output which you can see when it is running.
cclauss
Sep 30, 2020
Author
Member
Are you OK with the current state of this PR or is there something that I should change?
dhruvmanila
Sep 30, 2020
Member
I think validate_solutions.py should be in before_script section so that it fails fast when we remove || true
| install: pip install black flake8 | ||
| install: | ||
| - pip install --upgrade pip setuptools six | ||
| - pip install -r requirements_pytest.txt |
dhruvmanila
Sep 30, 2020
Member
By convention, those test requirements file are called requirements_dev.txt
dhruvmanila
Sep 30, 2020
Member
It's better to keep a separate file for test requirements for people who actually use it, they can install it directly. We can include all our test requirements in it, but it's up to you.
cclauss
Sep 30, 2020
Author
Member
I sense we have three sets of requirements:
- requirements.txt for our code's requirements
- requirements like black, flake8, isort that are embedded in the pre-commit yaml file
- the Travis CI requirements pytest and its plugins
I am reluctant to install requirements that we do not need both on contributors machines and on CI machines.
dhruvmanila
Sep 30, 2020
Member
pre-commitdoesn't require any installation, it is self-containedtest-requirements.txtis only for the user who wants to install all the test tools used in this repository- Travis CI won't install packages with
test-requirements.txt, we will specify which ones to install
How does this sound?
This means we will have only two requirements file:
- Code requirements
- Test requirements
cclauss
Sep 30, 2020
•
Author
Member
I believe that we are both good with our current requirements.txt.
That leaves open the question, what should go in the other Test requirements file. If I was running the tests on my machine, I would just type pytest because I do not need coverage and I do not need subtests (whatever that is). So, do we create a separate Test requirements file that only contains -r requirements.txt \n pytest?
I would prefer a called requirements_dev.txt or requirements_test.txt because:
- we have no files in this repo that have "
-" in their filenames - all the requirements files should appear next to each other in a code listing.
dhruvmanila
Sep 30, 2020
Member
flake8, black, isort, codespell fall in the requirements_dev.txt file as those are the tools used to develop the code as well in terms of linting and formating.
We will tell Travis to install requirements.txt and pytest and its plugins.
pytest-subtests are used when you're having tests for multiple iterations in a given function. By default, one function equals one test for pytest but if the function contains loops that iterate over many values, those will be considered as different tests. pytest will stop if any of those loops fail and exit immediately saying that the function failed. We want to know from all the iterations which ones are going to fail. I don't know if I can explain properly on text so you would have to do some research
dhruvmanila
Sep 30, 2020
Member
This is also one of the benefits of using pipenv for managing your environments. It separates out your common packages and dev packages and you don't have to worry about anything but that's a discussion for another day.
|
Both Travis runs execute in parallel in approx 3 min 30 sec. Awesome work @dhruvmanila |
|
Thank you! |
Describe your change:
Many tests (flake8, black, valid filenames, etc.) have migrated to a pre-commit hook #2511 and GitHub Action to run it #2515.
We can now remove those tests from Travis CI so that our automated tests run even faster. @dhruvmanila
Checklist:
Fixes: #{$ISSUE_NO}.