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

add options to ignore failing tests in prerelease #363

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mathias-luedtke
Copy link
Member

closes #362

@wxmerkt: With this fix you can set ABORT_ON_TEST_FAILURE_UNDERLAY=0 to make your test pass

@mathias-luedtke mathias-luedtke changed the base branch from master to legacy May 29, 2019 11:08
Copy link
Member

@130s 130s left a comment

Choose a reason for hiding this comment

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

Can we document these 2 variables new to ICI?

Other than that this LGTM. A clarification: What happens when a test fails while neither of ABORT_ON_TEST_FAILURE_{Over, UNDER}LAY are passed (i.e. default behavior)? If this changes the default, that might be noted in the document?

@mathias-luedtke mathias-luedtke changed the base branch from legacy to master July 14, 2019 13:02
@mathias-luedtke
Copy link
Member Author

This should het merged into the new version.

Can we document these 2 variables new to ICI?

I will add some line to the documentation of the prerelease tests.
They have been defined like this in ros_buildfarm.

What happens when a test fails while neither of ABORT_ON_TEST_FAILURE_{Over, UNDER}LAY are passed (i.e. default behavior)? If this changes the default, that might be noted in the document?

TL;DR: This patch does not change the default of industrial_ci.

In ros_buildfarm these variables default to 0, so failing tests are treated as errors.
In industrial_ci their default is 1. This PR adds an opt-out for this choice.

@130s
Copy link
Member

130s commented Dec 25, 2019

Is there any update?

I might be willing to add documentation. As a "new user" to the recently added changes, I might be able to capture missing info well enough. If that's ok in general, I'm fine in moving forward in merging w/o including more documentation (in that case create follow-up ticket and assign me/anyone appropriate).

@mathias-luedtke
Copy link
Member Author

mathias-luedtke commented Dec 25, 2019

Is there any update?

This needs to be rebased, but it is prioritized

Some notes on these flags could be added here:
https://github.com/ros-industrial/industrial_ci/blob/master/doc/index.rst#run-ros-prerelease-test
Just to comment on the non-default behavior, which is (examle)

By default this script will continue even if tests fail.
If you want the script to abort and return a non-zero return code
you can set the environment variable ABORT_ON_TEST_FAILURE=1.
You can also set ABORT_ON_TEST_FAILURE_UNDERLAY=1 or
ABORT_ON_TEST_FAILURE_OVERLAY=1 to only affect a specific workspace.

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