Travis CI: Remove redundant tests #2523
Conversation
.travis.yml
Outdated
| - 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/ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why do we run that project_euler/validate_solutions.py thru pytest? It seems like a lot of complexity and overhead.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Are you OK with the current state of this PR or is there something that I should change?
There was a problem hiding this comment.
I think validate_solutions.py should be in before_script section so that it fails fast when we remove || true
.travis.yml
Outdated
| install: pip install black flake8 | ||
| install: | ||
| - pip install --upgrade pip setuptools six | ||
| - pip install -r requirements_pytest.txt |
There was a problem hiding this comment.
By convention, those test requirements file are called requirements_dev.txt
There was a problem hiding this comment.
That file was not needed after all.
There was a problem hiding this comment.
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. 😁
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
cclauss
force-pushed
the
Travis-on-a-diet
branch
from
79754fe to
c3439a1
Compare
cclauss
force-pushed
the
Travis-on-a-diet
branch
from
46a147b to
12d2c07
Compare
cclauss
force-pushed
the
Travis-on-a-diet
branch
from
0645230 to
001040d
Compare
|
Both Travis runs execute in parallel in approx 3 min 30 sec. Awesome work @dhruvmanila |
|
Thank you! 😁 🎉 👍 |
* Travis CI: Remove redundant tests
* fixup! before_script
* updating DIRECTORY.md
Co-authored-by: github-actions <${GITHUB_ACTOR}@users.noreply.github.com>
* Travis CI: Remove redundant tests
* fixup! before_script
* updating DIRECTORY.md
Co-authored-by: github-actions <${GITHUB_ACTOR}@users.noreply.github.com>
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}.