You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by Javen O'Neal <ja...@gmail.com> on 2016/01/02 23:19:54 UTC
Detecting when ignored unit tests pass
We currently add @Ignore to failing unit tests so they don't cause our
builds to fail.
At some point in the future, we have to remember that a unit test was
written for a bug and remove the @Ignore.
Instead, we could write the unit test to pass with the current broken
behavior, and fail if the correct behavior is observed. The fail message
could be "If this test passes, it means Bug 12345 is resolved. Please
change this unit test to pass for correct behavior and close the bug."
Does this seem like a good idea? Is there a way we can avoid wrapping
failing unit tests in try/catch blocks to handle this, instead using some
annotation like @OpenBug(bug=12345, openIfThrows=[IllegalStateException]),
which fails with above message if an Exception class in openIfThrows is not
thrown. Converting this unit test to work for broken behavior to correct
behavior only requires removing the annotation.
There are a couple OpenBugzillaIssues.java unit test suites, which probably
allow non-Ignored tests to fail without breaking the build, but doesn't
alert us when a bug passes without reviewing the logs.
Re: Detecting when ignored unit tests pass
Posted by Dominik Stadler <do...@gmx.at>.
Hi,
I think this approach should work fine.
Dominik.
On Sun, Jan 3, 2016 at 10:30 AM, Javen O'Neal <ja...@gmail.com> wrote:
> Here's a working example of what I was talking about. This will break
> the build as soon as the bug is patched (prompting a unit test
> update). Because this unit test is run, any side-effects it has also
> occur, which differs from not running the unit test altogether. In
> this case, the side effect is modifying
> /test-data/slideshow/SampleShow.pptx.
>
>
> https://svn.apache.org/viewvc/poi/trunk/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFSlideShowFactory.java?r1=1722707&r2=1722706&pathrev=1722707
>
> public final class TestXSLFSlideShowFactory extends
> BaseTestSlideShowFactory {
> private static final String removeExpectedExceptionMsg =
> "This functionality this unit test is trying to test is
> now passing. " +
> "The unit test needs to be updated by deleting the
> expected exception code. Status and close any related bugs.";
>
> @Rule
> public ExpectedException thrown = ExpectedException.none();
>
> @Test
> public void testFactoryFromFile() throws Exception {
> // Remove thrown.* when bug 58779 is resolved
> thrown.expect(AssertionError.class);
> thrown.expectMessage("SampleShow.pptx sample file was modified
> as a result of closing the slideshow");
> thrown.reportMissingExceptionWithMessage("Bug 58779: " +
> removeExpectedExceptionMsg);
>
> testFactoryFromFile(filename);
> }
> }
>
> On Sat, Jan 2, 2016 at 5:11 PM, Javen O'Neal <ja...@gmail.com> wrote:
> > On Sat, Jan 2, 2016 at 2:54 PM, Dominik Stadler <do...@gmx.at>
> wrote:
> >> Adding a test-suite which looks at bugzilla will cause additional
> >> web-accesses during CI runs and might cause unstability if bugzilla is
> >> down/unreachable/flaky or changes web-address (just happened a year
> >> ago!)... I saw this being used locally at my company but am not sure if
> it
> >> is a good idea for an open source project where code is built all over
> the
> >> place.
> >
> > I was thinking of mentioning the bug number in the error message, but
> > assume the user will figure out what the bug URL is and change the
> > status as needed. No need to integrate bugzilla with the test suite.
> > Unit tests should be able to be run without an internet connection. If
> > the test is named something like "test12345" or "bug12345", someone
> > might be able to figure out what this is about--but we need to be
> > crystal clear that if the test breaks, it's because the broken feature
> > now works as expected, not because a working feature broke, and to fix
> > the unit test rather than the Java code.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>
>
Re: Detecting when ignored unit tests pass
Posted by Javen O'Neal <ja...@gmail.com>.
Here's a working example of what I was talking about. This will break
the build as soon as the bug is patched (prompting a unit test
update). Because this unit test is run, any side-effects it has also
occur, which differs from not running the unit test altogether. In
this case, the side effect is modifying
/test-data/slideshow/SampleShow.pptx.
https://svn.apache.org/viewvc/poi/trunk/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFSlideShowFactory.java?r1=1722707&r2=1722706&pathrev=1722707
public final class TestXSLFSlideShowFactory extends BaseTestSlideShowFactory {
private static final String removeExpectedExceptionMsg =
"This functionality this unit test is trying to test is
now passing. " +
"The unit test needs to be updated by deleting the
expected exception code. Status and close any related bugs.";
@Rule
public ExpectedException thrown = ExpectedException.none();
@Test
public void testFactoryFromFile() throws Exception {
// Remove thrown.* when bug 58779 is resolved
thrown.expect(AssertionError.class);
thrown.expectMessage("SampleShow.pptx sample file was modified
as a result of closing the slideshow");
thrown.reportMissingExceptionWithMessage("Bug 58779: " +
removeExpectedExceptionMsg);
testFactoryFromFile(filename);
}
}
On Sat, Jan 2, 2016 at 5:11 PM, Javen O'Neal <ja...@gmail.com> wrote:
> On Sat, Jan 2, 2016 at 2:54 PM, Dominik Stadler <do...@gmx.at> wrote:
>> Adding a test-suite which looks at bugzilla will cause additional
>> web-accesses during CI runs and might cause unstability if bugzilla is
>> down/unreachable/flaky or changes web-address (just happened a year
>> ago!)... I saw this being used locally at my company but am not sure if it
>> is a good idea for an open source project where code is built all over the
>> place.
>
> I was thinking of mentioning the bug number in the error message, but
> assume the user will figure out what the bug URL is and change the
> status as needed. No need to integrate bugzilla with the test suite.
> Unit tests should be able to be run without an internet connection. If
> the test is named something like "test12345" or "bug12345", someone
> might be able to figure out what this is about--but we need to be
> crystal clear that if the test breaks, it's because the broken feature
> now works as expected, not because a working feature broke, and to fix
> the unit test rather than the Java code.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: Detecting when ignored unit tests pass
Posted by Javen O'Neal <ja...@gmail.com>.
On Sat, Jan 2, 2016 at 2:54 PM, Dominik Stadler <do...@gmx.at> wrote:
> Adding a test-suite which looks at bugzilla will cause additional
> web-accesses during CI runs and might cause unstability if bugzilla is
> down/unreachable/flaky or changes web-address (just happened a year
> ago!)... I saw this being used locally at my company but am not sure if it
> is a good idea for an open source project where code is built all over the
> place.
I was thinking of mentioning the bug number in the error message, but
assume the user will figure out what the bug URL is and change the
status as needed. No need to integrate bugzilla with the test suite.
Unit tests should be able to be run without an internet connection. If
the test is named something like "test12345" or "bug12345", someone
might be able to figure out what this is about--but we need to be
crystal clear that if the test breaks, it's because the broken feature
now works as expected, not because a working feature broke, and to fix
the unit test rather than the Java code.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: Detecting when ignored unit tests pass
Posted by Dominik Stadler <do...@gmx.at>.
Hi,
Yes, sounds useful, previously we used Test*UnfixedBugs classes for things
that are not fixed yet. These are excluded in the CI runs, but we still
only find out about fixed items when manually running this test-class. I
just found one today which passed and indicated that an open bug was fixed
in the meantime.
Junit has some support for expected exceptions, but it is fairly poor in
features, but it may be sufficient for this use case.
Adding a test-suite which looks at bugzilla will cause additional
web-accesses during CI runs and might cause unstability if bugzilla is
down/unreachable/flaky or changes web-address (just happened a year
ago!)... I saw this being used locally at my company but am not sure if it
is a good idea for an open source project where code is built all over the
place.
Dominik.
On Sat, Jan 2, 2016 at 11:19 PM, Javen O'Neal <ja...@gmail.com> wrote:
> We currently add @Ignore to failing unit tests so they don't cause our
> builds to fail.
>
> At some point in the future, we have to remember that a unit test was
> written for a bug and remove the @Ignore.
>
> Instead, we could write the unit test to pass with the current broken
> behavior, and fail if the correct behavior is observed. The fail message
> could be "If this test passes, it means Bug 12345 is resolved. Please
> change this unit test to pass for correct behavior and close the bug."
>
> Does this seem like a good idea? Is there a way we can avoid wrapping
> failing unit tests in try/catch blocks to handle this, instead using some
> annotation like @OpenBug(bug=12345, openIfThrows=[IllegalStateException]),
> which fails with above message if an Exception class in openIfThrows is not
> thrown. Converting this unit test to work for broken behavior to correct
> behavior only requires removing the annotation.
>
> There are a couple OpenBugzillaIssues.java unit test suites, which probably
> allow non-Ignored tests to fail without breaking the build, but doesn't
> alert us when a bug passes without reviewing the logs.
>