threads / patch / 19162

patch, 6 partsBROKEN: Add a zero line context test for diff.DiffFormatter

Subject: [JGIT PATCH 7/6] BROKEN: Add a zero line context test for diff.DiffFormatter

## tl;dr

8 messages between May 3, 2009 and May 5, 2009. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

Shawn O. Pearce· May 3, 2009, 00:05 UTC · lore
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 This test currently fails because it shows the difference between
 the way CGit and JGit number a zero context line patch.
 Either we hack JGit to match CGit here, or we modify the test
 vector, or CGit agrees there's a bug and fixes their code, and we
 modify the test vector.
 .../org/spearce/jgit/diff/testContext0.out         |   18 ++++++++++++++++++
 .../spearce/jgit/diff/DiffFormatterReflowTest.java |    6 ++++++
 2 files changed, 24 insertions(+), 0 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst-rsrc/org/spearce/jgit/diff/testContext0.out
Show changes to 2 files +23 −0

org.spearce.jgit.test/tst-rsrc/org/spearce/jgit/diff/testContext0.out, org.spearce.jgit.test/tst/org/spearce/jgit/diff/DiffFormatterReflowTest.java

diff --git a/org.spearce.jgit.test/tst-rsrc/org/spearce/jgit/diff/testContext0.out b/org.spearce.jgit.test/tst-rsrc/org/spearce/jgit/diff/testContext0.out
new file mode 100644
index 0000000..d36e3fa
--- /dev/null
+++ b/org.spearce.jgit.test/tst-rsrc/org/spearce/jgit/diff/testContext0.out
@@ -0,0 +1,18 @@
+diff --git a/X b/X
+index a3648a1..2d44096 100644
+--- a/X
++++ b/X
+@@ -2,0 +3 @@
++c
+@@ -17,2 +17,0 @@
+-r
+-s
+@@ -23,2 +22,6 @@
+-x
+-y
++0
++1
++2
++3
++4
++5
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/diff/DiffFormatterReflowTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/diff/DiffFormatterReflowTest.java
index f47282c..5d2ee40 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/diff/DiffFormatterReflowTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/diff/DiffFormatterReflowTest.java
@@ -74,6 +74,12 @@ public void testNegativeContextFails() throws IOException {
 		}
 	}
 
+	public void testContext0() throws IOException {
+		init("X");
+		fmt.setContext(0);
+		assertFormatted();
+	}
+
 	public void testContext1() throws IOException {
 		init("X");
 		fmt.setContext(1);

-- 
1.6.3.rc4.190.g4648
Shawn O. Pearce· May 3, 2009, 00:14 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 8/6] Fix zero context insert and delete hunk headers to match CGit

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
  "Shawn O. Pearce" <spearce@spearce.org> wrote:
  >  This test currently fails because it shows the difference between
  >  the way CGit and JGit number a zero context line patch.
  > 
  >  Either we hack JGit to match CGit here
  And here is that hack.  It just feels wrong to me that I need
  to subtract 1 from the Edit region's line numbers, *only* when
  context is 0, in order to get the same output as CGit.
 .../src/org/spearce/jgit/diff/DiffFormatter.java   |   23 ++++++++++++++++---
 1 files changed, 19 insertions(+), 4 deletions(-)
Show changes to org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java +19 −4
diff --git a/org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java b/org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java
index 97db9a2..9930904 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/diff/DiffFormatter.java
@@ -120,7 +120,7 @@ private void formatEdits(final OutputStream out, final RawText a,
 			final int aEnd = Math.min(a.size(), endEdit.getEndA() + context);
 			final int bEnd = Math.min(b.size(), endEdit.getEndB() + context);
 
-			writeHunkHeader(out, aCur, aEnd, bCur, bEnd, curIdx == 0);
+			writeHunkHeader(out, aCur, aEnd, bCur, bEnd, curEdit, curIdx == 0);
 
 			while (aCur < aEnd || bCur < bEnd) {
 				if (aCur < curEdit.getBeginA() || endIdx + 1 < curIdx) {
@@ -141,9 +141,24 @@ private void formatEdits(final OutputStream out, final RawText a,
 		}
 	}
 
-	private void writeHunkHeader(final OutputStream out, final int aCur,
-			final int aEnd, final int bCur, final int bEnd,
-			final boolean firstHunk) throws IOException {
+	private void writeHunkHeader(final OutputStream out, int aCur, int aEnd,
+			int bCur, int bEnd, final Edit curEdit, final boolean firstHunk)
+			throws IOException {
+		if (context == 0) {
+			switch (curEdit.getType()) {
+			case INSERT:
+				aCur--;
+				aEnd--;
+				break;
+			case DELETE:
+				bCur--;
+				bEnd--;
+				break;
+			default:
+				break;
+			}
+		}
+
 		out.write('@');
 		out.write('@');
 		if (firstHunk) {

-- 
1.6.3.rc4.190.g4648
Miles Bader· May 3, 2009, 00:50 UTC · re: Shawn O. Pearce · lore

Re: [JGIT PATCH 8/6] Fix zero context insert and delete hunk headers to match CGit

"Shawn O. Pearce" <spearce@spearce.org> writes:
>   And here is that hack.  It just feels wrong to me that I need
>   to subtract 1 from the Edit region's line numbers, *only* when
>   context is 0, in order to get the same output as CGit.

A big comment saying "this may look a bit funny, but it's the standard's fault: [text from std...]" might help salve the wound...

-Miles
-- 
.Numeric stability is probably not all that important when you're guessing.
Robin Rosenberg· May 3, 2009, 08:25 UTC · re: Miles Bader · lore

Re: [JGIT PATCH 8/6] Fix zero context insert and delete hunk headers to match CGit

söndag 03 maj 2009 02:50:36 skrev Miles Bader <miles@gnu.org>:
Show 7 quoted lines
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> >   And here is that hack.  It just feels wrong to me that I need
> >   to subtract 1 from the Edit region's line numbers, *only* when
> >   context is 0, in order to get the same output as CGit.
> 
> A big comment saying "this may look a bit funny, but it's the standard's
> fault: [text from std...]" might help salve the wound...
1) I agree. This need to be commented in the code.
2) Do we need to fix it? I.e. is there a problem or just two different results,
where one is correct, the other incorrect but harmless.
3) We should have a convention like C Git for marking known breakages.
One option is FIXME, another it so go JUnit 4 and abuse the expected exception 
annotation (using it for declaring OK exceptions is pretty bad use anyway I think,
so we might use it for something better), or perhaps the @Ignore annotation which
is meant specifically for this and other cases. A FIXME can be implemented right
away.
-- robin
Ferry Huberts (Pelagic)· May 3, 2009, 08:31 UTC · re: Robin Rosenberg · lore

Re: [JGIT PATCH 8/6] Fix zero context insert and delete hunk headers to match CGit

Show 6 quoted lines
> 3) We should have a convention like C Git for marking known breakages.
> One option is FIXME, another it so go JUnit 4 and abuse the expected exception 
> annotation (using it for declaring OK exceptions is pretty bad use anyway I think,
> so we might use it for something better), or perhaps the @Ignore annotation which
> is meant specifically for this and other cases. A FIXME can be implemented right
> away.

standard pratice for junit would be to write a test case on what you would expect to be _correct_ behaviour. obviously that test would then fail. it would be a know failure in the test suite. do not go ignoring it. it's better to keep being reminded that stuff doesn't work :-)

Robin Rosenberg· May 3, 2009, 09:24 UTC · re: Ferry Huberts (Pelagic) · lore

Re: [JGIT PATCH 8/6] Fix zero context insert and delete hunk headers to match CGit

söndag 03 maj 2009 10:31:30 skrev "Ferry Huberts (Pelagic)" <ferry.huberts@pelagic.nl>:
Show 11 quoted lines
> > 3) We should have a convention like C Git for marking known breakages.
> > One option is FIXME, another it so go JUnit 4 and abuse the expected exception 
> > annotation (using it for declaring OK exceptions is pretty bad use anyway I think,
> > so we might use it for something better), or perhaps the @Ignore annotation which
> > is meant specifically for this and other cases. A FIXME can be implemented right
> > away.
> 
> standard pratice for junit would be to write a test case on what you would 
> expect to be _correct_ behaviour. obviously that test would then fail.
> it would be a know failure in the test suite. do not go ignoring it. it's 
> better to keep being reminded that stuff doesn't work :-)
What I've see so far is that people start ignoring almost any failure, including new ones, when the test suites contains fails with "known" failues. The assumption is that the failed tests were the same as before.

Worse, automated tests have a hard time telling the difference. Currently I ran the jgit tests as part of the Eclipse plugin build and I want it to stop if there is a problem that we don't know of.

"Annotation" of different kinds can be "grepped" for so we can find the broken cases separately and even refuse completion of release builds if we decide on that.

Our primary UI right now is the Eclipse JUnit tests runner and I don't want to be remined of Shawn's or whoever's bugs when trying to make sure I don't break anything. Red = *I* broke something or found something new.

TestNG has a nice way of classifying tests so, we could mark failures as "known failures" and specifically exclude/include them when invoking the JUnit tests.

Best is to fix before we apply the patches as happened this time. So this problem still remains theoretical :)
-- robin
-- robin
SZEDER Gábor· May 5, 2009, 22:19 UTC · re: Robin Rosenberg · lore

Re: [JGIT PATCH 8/6] Fix zero context insert and delete hunk headers to match CGit

Hi,
On Sun, May 03, 2009 at 11:24:07AM +0200, Robin Rosenberg wrote:
Show 28 quoted lines
> söndag 03 maj 2009 10:31:30 skrev "Ferry Huberts (Pelagic)" <ferry.huberts@pelagic.nl>:
> > > 3) We should have a convention like C Git for marking known breakages.
> > > One option is FIXME, another it so go JUnit 4 and abuse the expected exception 
> > > annotation (using it for declaring OK exceptions is pretty bad use anyway I think,
> > > so we might use it for something better), or perhaps the @Ignore annotation which
> > > is meant specifically for this and other cases. A FIXME can be implemented right
> > > away.
> > 
> > standard pratice for junit would be to write a test case on what you would 
> > expect to be _correct_ behaviour. obviously that test would then fail.
> > it would be a know failure in the test suite. do not go ignoring it. it's 
> > better to keep being reminded that stuff doesn't work :-)
> 
> What I've see so far is that people start ignoring almost any failure, including new ones, when the test suites contains fails with "known" failues. The assumption is that the failed tests were the same as before.
> 
> Worse, automated tests have a hard time telling the difference. Currently I ran
> the jgit tests as part of the Eclipse plugin build and I want it to stop if there is a problem that we don't know of. 
> 
> "Annotation" of different kinds can be "grepped" for so we can find the broken
> cases separately and even refuse completion of release builds if we decide
> on that. 
> 
> Our primary UI right now is the Eclipse JUnit tests runner and I don't want
> to be remined of Shawn's or whoever's bugs when trying to make sure I don't
> break anything. Red = *I* broke something or found something new. 
> 
> TestNG has a nice way of classifying tests so, we could mark failures as "known failures" and specifically exclude/include them when invoking the
> JUnit tests.

you could use test suites to easily circumvent this in JUnit, even in JUnit 3.x.

Just set up two test suites: one for the tests that should pass and one for the tests with known breakages. That way you can run either only the "good" tests or only the broken ones. Or even both, and you can easily discern failures caused by known breakages from your new breakages by looking at the tree of tests in eclipse's JUnit view.

Regards, Gábor

← back to recent threads