Add testing script for local lint and python test. #3797
Conversation
| set +x | ||
|
|
||
| # Assume the pwd is under models. | ||
| cd official |
There was a problem hiding this comment.
cd "$(git rev-parse --show-toplevel)/official" is a bit more flexible, as you can be anywhere in the project.
There was a problem hiding this comment.
I am wary of adding git magic into the script. Maybe instructions above with the alias lint_and_test, along with setting $TF_MODELS_ROOT or something to that effect, which, if empty, will mean you cd into just official as now, but, if set, the right place.
There was a problem hiding this comment.
Done. Changed to check the file location of the script and cd into the relative directory of the git repository root.
| # Check lint: | ||
| RC_FILE="utils/testing/pylint.rcfile" | ||
|
|
||
| echo "===========Running lint test============" |
There was a problem hiding this comment.
git diff --name-only origin/master ./official/ has the nice property that you only have to care about stuff you touched. (And it makes linting faster.) The one caveat is it includes deleted files so you would have to do an existence test.
There was a problem hiding this comment.
If there are errors in things you didn't touch, the tests on Kokoro will fail as well. Printing them all will make us fix them faster. One hopes.
There was a problem hiding this comment.
Yea, currently running all the tests rather than the touched one. Since the file depend on each other, you cannot tell whether the test will break or not even its not touched.
| echo "===========Running Python test============" | ||
|
|
||
| for test_file in `find . -name '*test.py' -print` | ||
| do |
There was a problem hiding this comment.
May as well run in parallel. I looks like xargs can do it; otherwise it's pretty trivial to do in python with subprocess. The reason being...
There was a problem hiding this comment.
It's worth noting that even our unit tests take a long time now. How long will this take locally? In my experience, once testing takes longer than ~5 min, people stop doing it. That said, I don't want to add a ton of logic here that has to be separately maintained from the Kokoro files. In fact, I already feel nervous about this duplication. It's easy to imagine the two sets of files diverging. I don't have a solution there, but... maybe the better way to handle this is a private wrapper around the kokoro files. That way, we can include git magic, whatever you want. Runs from the internal files, on external files.
There was a problem hiding this comment.
In total it's something like 7-8 min. (So ~15 if you do py2 and py3) I agree that likelihood of being run goes down with run time.
As far as code duplication, what if the heavy lifting is in a public script and Kokoro's build.sh just finds and runs that script? (At least for testing and linting.) I think this is the same as what Karmel is suggesting?
There was a problem hiding this comment.
The updated version of script will the one for all. Kokoro tests will just call it.
| for test_file in `find . -name '*test.py' -print` | ||
| do | ||
| echo "Testing ${test_file}" | ||
| python "${test_file}" || exit_code=$? |
There was a problem hiding this comment.
... this should probably do py2 and py3.
| @@ -0,0 +1,43 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
It would be nice to provide a command to run this from anywhere in the repo. For instance:
alias lint_and_test=$(git rev-parse --show-toplevel)/official/utils/testing/scripts/presubmit.sh
Either in the README or in this file.
There was a problem hiding this comment.
Have usage example on the top of the script.
| for file in `find . -name '*.py' ! -name '*test.py' -print` | ||
| do | ||
| echo "Linting ${file}" | ||
| pylint --rcfile="${RC_FILE}" "${file}" || exit_code=$? |
There was a problem hiding this comment.
If you change the cd at the top, you should also inject the absolute path into the rcfile path. It has trouble finding it otherwise. (Not sure if there is some weird cd related bash behavior.)
| set +x | ||
|
|
||
| # Assume the pwd is under models. | ||
| cd official |
There was a problem hiding this comment.
I am wary of adding git magic into the script. Maybe instructions above with the alias lint_and_test, along with setting $TF_MODELS_ROOT or something to that effect, which, if empty, will mean you cd into just official as now, but, if set, the right place.
| # Check lint: | ||
| RC_FILE="utils/testing/pylint.rcfile" | ||
|
|
||
| echo "===========Running lint test============" |
There was a problem hiding this comment.
If there are errors in things you didn't touch, the tests on Kokoro will fail as well. Printing them all will make us fix them faster. One hopes.
| echo "===========Running Python test============" | ||
|
|
||
| for test_file in `find . -name '*test.py' -print` | ||
| do |
There was a problem hiding this comment.
It's worth noting that even our unit tests take a long time now. How long will this take locally? In my experience, once testing takes longer than ~5 min, people stop doing it. That said, I don't want to add a ton of logic here that has to be separately maintained from the Kokoro files. In fact, I already feel nervous about this duplication. It's easy to imagine the two sets of files diverging. I don't have a solution there, but... maybe the better way to handle this is a private wrapper around the kokoro files. That way, we can include git magic, whatever you want. Runs from the internal files, on external files.
1. Check the script file location and cd into repo root dir. 2. Allow caller to call differnt tests.
qlzh727
changed the title
|
Done. The script has been updated so that it can be run both on Kokoro and local. We can update Kokoro scripts once this PR is committed. |
|
The tests are still serial though, right? |
|
Correct. Running tests in parallel might generate confusing error logs since they are mixed together. On the other hand, Bazel is helpful for those case, since it generate a summary at the end the run. |
|
Discussed offline. I am fine with the plan of "serial for now, Bazel eventually" rather than have a somewhat complicated python script that we'll eventually get rid of anyway. |
* Add presubmit testing script for local testing. * Update the test script to be more modularized. 1. Check the script file location and cd into repo root dir. 2. Allow caller to call differnt tests.
No description provided.