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

Improve test failure directions #4902

Merged
merged 7 commits into from
Apr 7, 2020
Merged

Conversation

blag
Copy link
Contributor

@blag blag commented Apr 4, 2020

Updating dependencies is not an obvious, straightforward, or documented process, so many people have tried to simply update the root requirements.txt files, or the requirements.txt in one of our subpackages (eg: runners and st2* packages). While we do have headers on all of those files instructing people how to do it properly, it's very easy to miss them.

To combat this, I have previously added a step in the ci-checks that runs the make requirements target and uses git to check if there were any changes.

However, when this causes test failures in Travis CI, the error output isn't very helpful. Furthermore, running make requirements takes a long time to complete.

This PR splits the requirements target into requirements and .requirements and uses targets to maintain reverse compatibility. Additionally, it gives very obvious directions for how to update versions properly, and how to check again before pushing additional changes.

$ make .requirements .check-requirements

============== CHECKING REQUIREMENTS ==============

# Update requirements and then make sure no files were changed
git status -- *requirements.txt */*requirements.txt | grep -q "nothing to commit" || { \
	echo "It looks like you directly modified a requirements.txt file, an"; \
	echo "in-requirements.txt file, or fixed-requirements.txt without running:"; \
	echo ""; \
	echo "    make .requirements"; \
	echo ""; \
	echo "Please update all of the requirements.txt files by running that command"; \
	echo "and committing all of the changed files. You can quickly check the results"; \
	echo "with:"; \
	echo ""; \
	echo "    make .check-requirements"; \
	echo ""; \
	exit 1; \
}
It looks like you directly modified a requirements.txt file, an
in-requirements.txt file, or fixed-requirements.txt without running:

    make .requirements

Please update all of the requirements.txt files by running that command
and committing all of the changed files. You can quickly check the results
with:

    make .check-requirements

Makefile:165: recipe for target '.check-requirements' failed
make: *** [.check-requirements] Error 1

Additionally, since I didn't realize that we had a similar situation with scripts/dist_utils.py and didn't update those for #4895, this PR adds a similar check for the sdist-requirements, with similar instructions, and a similar make target.

$ make .sdist-requirements .check-sdist-requirements
# Copy over shared dist utils module which is needed by setup.py
# Copy over CHANGELOG.RST, CONTRIBUTING.RST and LICENSE file to each component directory
#@for component in st2actions st2api st2auth st2client st2common st2debug st2exporter st2reactor st2stream contrib/runners/mistral_v2 contrib/runners/noop_runner contrib/runners/inquirer_runner contrib/runners/local_runner contrib/runners/winrm_runner contrib/runners/http_runner contrib/runners/remote_runner contrib/runners/orquesta_runner contrib/runners/python_runner contrib/runners/announcement_runner contrib/runners/action_chain_runner; do\
#	test -s $component/README.rst || cp -f README.rst $component/; \
#	cp -f CONTRIBUTING.rst $component/; \
#	cp -f LICENSE $component/; \
#done

============== CHECKING SDIST REQUIREMENTS ==============

# Update requirements and then make sure no files were changed
git status -- */dist_utils.py contrib/runners/*/dist_utils.py | grep -q "nothing to commit" || { \
	echo "It looks like you directly modified a dist_utils.py, or the source "; \
	echo "scripts/dist_utils.py file without running:"; \
	echo ""; \
	echo "    make .sdist-requirements"; \
	echo ""; \
	echo "Please update all of the dist_utils.py files by running that command"; \
	echo "and committing all of the changed files. You can quickly check the results"; \
	echo "with:"; \
	echo ""; \
	echo "    make .check-sdist-requirements"; \
	echo ""; \
	exit 1; \
}
It looks like you directly modified a dist_utils.py, or the source
scripts/dist_utils.py file without running:

    make .sdist-requirements

Please update all of the dist_utils.py files by running that command
and committing all of the changed files. You can quickly check the results
with:

    make .check-sdist-requirements

Makefile:190: recipe for target '.check-sdist-requirements' failed
make: *** [.check-sdist-requirements] Error 1

@blag blag added this to the 3.2.0 milestone Apr 4, 2020
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Apr 4, 2020
Copy link
Member

@nmaludy nmaludy left a comment

Choose a reason for hiding this comment

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

Love this!

@blag blag merged commit 18c4a0e into master Apr 7, 2020
@blag blag deleted the better-test-failure-directions branch April 7, 2020 17:29
Copy link
Member

@punkrokk punkrokk left a comment

Choose a reason for hiding this comment

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

Great find

Comment on lines +522 to +525
$(VIRTUALENV_DIR)/bin/pip install --upgrade "pip>=19.3.1"
# setuptools >= 41.0.1 is required for packs.install in dev envs
# setuptools >= 42 is required so setup.py install respects dependencies' python_requires
$(VIRTUALENV_DIR)/bin/pip install --upgrade "setuptools>=42"
Copy link
Member

@arm4b arm4b Apr 7, 2020

Choose a reason for hiding this comment

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

It looks like with this diff we just relaxed again pinned versions set before

pip==20.0.2
setuptools==44.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement infrastructure: ci/cd size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants