-
Notifications
You must be signed in to change notification settings - Fork 224
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
allure-testng: add property to disable configuration failures logging #920
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Please add a test that will check that only one skipped result is provided in case there is a configuration failure, and the new setting is set.
allure-testng/src/main/java/io/qameta/allure/testng/config/AllureTestNgConfig.java
Outdated
Show resolved
Hide resolved
New unit test added that asserts when setting is used, only the main test shows as skipped, and not the configuration |
Didn't mean to close the PR, mouse being funny clicked the wrong button near the [Comment] |
@baev The PR is now ready for review and should be ready to merge afterwards. Just awaiting your followup :) |
allure-testng/src/main/java/io/qameta/allure/testng/AllureTestNg.java
Outdated
Show resolved
Hide resolved
allure-testng/src/test/java/io/qameta/allure/testng/AllureTestNgTest.java
Outdated
Show resolved
Hide resolved
…/overloads to intake a config setting for tests
@baev Requested changes have been implemented, requesting another review. I took liberty to also fix another similar test to conform with the requested changes. |
allure-testng/src/test/java/io/qameta/allure/testng/AllureTestNgTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 great work. One more comment
allure-testng/src/main/java/io/qameta/allure/testng/config/AllureTestNgConfig.java
Outdated
Show resolved
Hide resolved
please, fix the build |
@baev Okay, for real this time, I think it should be fully ready to go. All wording was changed from log -> hide, and the default value is now False to match the other config property. Added @SuppressWarnings("PMD.NcssCount") so it should build now. Thanks for all the back and fourth I am about to go on an (mostly) offline vacation but will check back just in case anything else is needed. Looking forward to the release this gets added into :) |
Context
This will give users the choice of not logging @Before/@after methods that experience errors (fail/broken) as standalone methods in the report, as is the default behavior.
This PR enables configuration capabilities for the feature added in (#274). That feature has been requested to be modified or removed on a few occasions (#356 which this addresses and also in #316). With this change, you are still able to see the failure reason and stacktrace in the Skipped test's before stages and main fail cause if that information is needed, while keeping TestNG's behavior consistent with a failed configuration not resulting in a failed test case (Since no test ran).
By using allure.properties and setting 'allure.testng.log.configuration.failures=false' the default process of creating the 'fake test' for the SetUp/TearDown will be bypassed, resulting only in the original test showing as skipped.
This should allow for the report to more accurately reflect only test cases pass/fail/broken/skip, and not report on configuration issues, which can be very important in certain situations.
Checklist