[rfc][icedtea-web] known to fail annotation
Jiri Vanek
jvanek at redhat.com
Wed May 23 07:59:03 PDT 2012
On 05/22/2012 07:30 PM, Omair Majid wrote:
> On 05/22/2012 08:56 AM, Jiri Vanek wrote:
>> As agreed on IRC, Percentage removed. What do you think now?
>
> I am wondering what really differentiates KnownToFail from Ignore, aside
> from the fact that we are processing the KnownToFail annotation. But we
> could do the same for Ignore, right? Is it that tests makred with Ignore
> are not run while tests marked with KnownToFail are run?
Yes. This is crucial difference.
>
> Something I am unclear about. Say I have this test case:
>
> class TestCase {
> @Test
> @KnownToFail
> public void someTestMethod() throws Exception {
> assertTrue(false);
> }
> }
>
> What stat do you think should be printed?
>
> Passed: 0, Failed: 1, Ignored: 0
>
> or
>
> Passed: 0, Failed: 0, Ignored: 1
> (with something to indicate the exepected failure?)
Well this is correct question. You saw my outputs below. (to print it as failed, and print somewhere
that those are supposed to fail) Are you ok with them?
>
>> After deep meditation I consider it as better now too :)
>
> I am glad we came to the same conclusion :)
>
> I have some comments below, and it could use some cleanup in general.
>
>> diff -r 1088b2dffe49 tests/junit-runner/JunitLikeXmlOutputListener.java
>> --- a/tests/junit-runner/JunitLikeXmlOutputListener.java Tue May 22 12:08:17 2012 +0200
>> +++ b/tests/junit-runner/JunitLikeXmlOutputListener.java Tue May 22 14:32:43 2012 +0200
>
>> @@ -45,6 +47,7 @@
>> private static final String TEST_ELEMENT = "testcase";
>> private static final String BUGS = "bugs";
>> private static final String BUG = "bug";
>> + private static final String K2F = "knownToFail";
>
> A more xml-like version might be "known-to-fail" ;)
done (although I don`t like this convention ;)
>
>
>> private class ClassCounter {
>>
>> Class c;
>> int total;
>> int failed;
>> int passed;
>> + int ignored;
>
> Nice!
>
>> @@ -146,20 +160,53 @@
>> }
>>
>> @Override
>> + public void testIgnored(Description description) throws Exception {
>> + testDone(description, 0, 0, true);
>> + }
>> +
>> + @Override
>> public void testFinished(org.junit.runner.Description description) throws Exception {
>> - long testTime = System.nanoTime()/1000l/1000l - testStart;
>> + long testTime = System.nanoTime() / 1000l / 1000l - testStart;
>
> Try:
> long testTime = TimeUnit.NANOSECONDS.toMillis(System.nanoTime()) -
> testStart;
>
> But better would be to keep testStart in nanoseconds and do the
> conversion to seconds after the subtraction.
Done!
>
>
>> double testTimeSeconds = ((double) testTime) / 1000d;
>> + testDone(description, testTime, testTimeSeconds, false);
>> + }
>>
>> - Map<String, String> testcaseAtts = new HashMap<String, String>(3);
>> + private void testDone(Description description, long testTime, double testTimeSeconds, boolean ignored) throws Exception {
>> + Class q = null;
>> + Method qm = null;
>
> s/q/testClass/
> s/qm/testMethod/
>
>> + try {
>> + q = description.getTestClass();
>> + String qs = description.getMethodName();
>> + if (qs.contains(" - ")) {
>> + qs = qs.replaceAll(" - .*", "");
>> + }
>> + qm = q.getMethod(qs);
>
> I see this bit of code duplicated elsewhere too. Also, I find it bizarre
> that the method name contains a " - ". Can you move it to a separate
> method and/or add comments explaining this?
Ah. That is relict from @Browser development . I do them in parallel. Sorry for this. I have added
comment and it do no harm. If It will change in browser annotation then I will keep in mind to
remove this.
>
>> + } catch (Exception ex) {
>> + ex.printStackTrace();
>> + }
>> + Map<String, String> testcaseAtts = new HashMap<String, String>(4);
>> NumberFormat formatter = new DecimalFormat("#0.0000");
>> String stringedTime = formatter.format(testTimeSeconds);
>> stringedTime.replace(",", ".");
>
> This replacement of comma with a dot looks strange. It looks like
> something that should be handled by the localization code.
It is not related to this patch, I have not find quick way of fix. Adding to my todo!
>
>> try {
>> - Class q = description.getTestClass();
>> - String qs=description.getMethodName();
>> - if (qs.contains(" - ")) qs=qs.replaceAll(" - .*", "");
>> - Method qm = q.getMethod(qs);
>> - Bug b = qm.getAnnotation(Bug.class);
>> - if (b != null) {
>> - openElement(BUGS);
>> - String[] s = b.id();
>> - for (String string : s) {
>> - String ss[]=createBug(string);
>> - Map<String, String> visibleNameAtt=new HashMap<String, String>(1);
>> + if (q != null&& qm != null) {
>> + Bug b = qm.getAnnotation(Bug.class);
>> + if (b != null) {
>> + openElement(BUGS);
>> + String[] s = b.id();
>> + for (String string : s) {
>> + String ss[] = createBug(string);
>
> "b", "s", and "ss" are not very descriptive names. Can you make them
> slightly more descriptive?
done O:)
>
>> + Map<String, String> visibleNameAtt = new HashMap<String, String>(1);
>> visibleNameAtt.put("visibleName", ss[0]);
>> - openElement(BUG,visibleNameAtt);
>> + openElement(BUG, visibleNameAtt);
>> writer.write(ss[1]);
>> closeElement(BUG);
>> + }
>> + closeElement(BUGS);
>> }
>> - closeElement(BUGS);
>> }
>> } catch (Exception ex) {
>> ex.printStackTrace();
>> @@ -200,16 +254,31 @@
>> ClassCounter cc = classStats.get(description.getClassName());
>
> This is probably the best argument I can point to for renaming
> ClassCounter to ClassStat ;)
done - although did patch little bit messy.
>
> Anyway, please rename cc to "classStat" "testStat" or something.
done!
>
>> @@ -45,7 +60,56 @@
>> int passed = result.getRunCount() - result.getFailureCount() - result.getIgnoreCount();
>> int failed = result.getFailureCount();
>> int ignored = result.getIgnoreCount();
>> - writer.println("Test results: passed: " + passed + "; failed: " + failed + "; ignored: " + ignored);
>> + writer.println("Test results ("+result.getRunCount()+"): passed: " + passed + "; failed: " + failed + "; ignored: " + ignored);
>> + writer.println("Test known to fail ("+totalK2F+"): passed: " + passedK2F + "; failed: " + failedK2F + "; ignored: " + ignoredK2F);
>> + }
>
> Are the builders prepared to handle this change in output?
I have swap the lines so they will be affected with less probability. On my side (daily report) they
will be ok. Who should I notify/where what to verify for builders on classpath.org?
>
>
>> diff -r 1088b2dffe49 tests/report-styles/jreport.xsl
>> --- a/tests/report-styles/jreport.xsl Tue May 22 12:08:17 2012 +0200
>> +++ b/tests/report-styles/jreport.xsl Tue May 22 14:32:43 2012 +0200
>> @@ -53,11 +53,17 @@
>> <xsl:value-of select="/testsuite/date"/>
>> <br/>
>> <h2>Result: (<xsl:value-of select="round(sum(/testsuite/testcase/@time))"/>s)</h2>
>> +<h4>In brackets are KnownToFail values if any</h4>
>
> Might be nicer if we just leave this description out and display
> "expected:" or "known:" in the brackets.
hmhm.. Actually I don know... It it will not be nice to have
total 10 (from which 8 known to fail)
passed 5 (but 4 are know to fail)
failed 3 (and 3 known to fail)
ignored 2 (one known to fail)
To much words... But can be more descriptive. What do you think?
I left my original one in patch.
>
>> @@ -65,6 +71,11 @@
>> <div class="cell1">passed:</div>
>> <div class="cell2">
>> <xsl:value-of select="/testsuite/stats/summary/passed"/>
>> +<xsl:choose>
>> +<xsl:when test="/testsuite/stats/summary/passed/@knownToFail!=0">
>> + (<xsl:value-of select="/testsuite/stats/summary/passed/@knownToFail"/>)
>> +</xsl:when>
>> +</xsl:choose>
>
> I am having trouble parsing this. Are these tests that are known to fail
> that passed? Should we be flagging this more importantly?
>
This should not happen, but it can happe:). Eg some patch fill fix some secondary issue unwillingly,
so author will be noticed.
Or programmer just forget that some more tests corresponding to his patch have this annotation.
explained?
>> diff -r 1088b2dffe49 tests/netx/jnlp_testsengine/net/sourceforge/jnlp/annotations/KnownToFail.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/netx/jnlp_testsengine/net/sourceforge/jnlp/annotations/KnownToFail.java Tue May 22 14:32:43 2012 +0200
>
>> +/**
>> + * This annotation should be declared for each test which is representing an issue
>> + * where icedtea-web is not doing as correctly as expected, and so the test, by its failure,
>> + * is showing this behaviour.
>> + * If the issue is fixed, annotation should be removed.
>> + *
>> + */
>
> How about:
>
> """
> This annotation marks a test as a known failure (as opposed to a
> regression). A test that is a known failure will not hold of a release,
> nor should developers hold off a fix if they run the unit tests and a
> test marked as a known failure fails.
> <p>
> This annotation is meant for adding tests for bugs before the fix is
> implemented.
> """
done :)
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: KnownToFailAnnotation_3-all.diff
Type: text/x-patch
Size: 26979 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120523/02d6280b/KnownToFailAnnotation_3-all.diff
More information about the distro-pkg-dev
mailing list