[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
RE: [aspectj-dev] rc2 **Preview Build #4** and new failures (!)
|
Wes Isberg wrote:
> Sorry about wasting George's time by not emailing about those failures, if
> not fixing the tests.
> But my commits didn't break anything.
I have to disagree. Your commits broke two tests by changing the effective API of the test harness. These tests both expected the output directory to be on the classpath. They were completely reasonable and correct tests in the context in which they were written (which was prior to any incremental testing infrastructure).
You made a change to the test harness API that improved the value of the harness. This let you write two new tests that exposed incremental compilation errors that we couldn't see before. This is a great thing! However, whenever you make an API change (particularly this late in a release cycle) you should make sure that any clients of your API still work correctly.
I agree with you that in theory a change to the harness could reveal previously hidden bugs in the compiler and that in such a case you wouldn't be obligated to fix those bugs but only to bring them to the attention of this list. However, I think that such cases are rare. Any change that causes previously passing tests to fail should be a red flag. At the very least it should be cause for discussion on this list BEFORE committing to the tree to confirm that it is not a regression.
-Jim
> I did see those two test failures. The point of fixing the test harness
> was to show the cases
> hidden by the harness adding the output directory to the classpath.
> Currently, there is no way
> to tell the harness to use the output directory in a full compile from a
> previous compile, so
> the tests were relying on a bug. I will implement a flag for the
> <compile> step and
> fix this in the tests.
>
> That said, the harness is a diagnostic tool. I don't think fixing the
> harness mean you take
> responsibility for fixing all the bugs in the test cases or in the
> components under test.
> It's true no commits should be made that cause a regression, but this was
> instead a bug in
> the test case that needed to be seen. I disagree that adding the output
> directory back
> to the compile classpath was the right fix. The test author or someone
> who investigates
> could do that, but adding the output directory was a workaround for a bug
> in incremental
> compilation, not for a bug in the test case specification.
>
> However, when I checked in I should have noted those failures so everyone
> knew that
> they needed to be accounted for. I would amend the rule to say that any
> new failures in
> the tests shown by a commit should be noticed to the mail list. Before
> committing, you
> must fix any regressions - that is, bugs caused by your commits - but you
> need not fix
> bugs revealed by your commits. Showing previously-concealed bugs
> increases quality.
>
> Rules aside, here rather than just email or notice the messages were the
> same as for the
> inc-compile failure case, I could have seen that they were not inc-compile
> tests,
> and probably would have queried whether they depended on the output
> directory,
> saving George some time. Certainly good citizens would analyze if not fix
> the easy
> previously-concealed bugs, as I should have.
>
> Wes
>
> Jim.Hugunin@xxxxxxxx wrote:
>
> > George, you made the right quick fix here. These two (non-incremental)
> test cases rely on the output directory being on the classpath for the
> second of their two compiles. Wes's change last night broke this and he
> should have caught the problem by running the test suite BEFORE the
> commit.
> >
> > Continuing our process of clarifying our developer policies as we go,
> here's another one that I assumed was obvious:
> >
> > * cvs commits should only be made after running the test suite -- this
> includes running the full harness tests in at least one configuration and
> running the junit tests for any module that you touch
> >
> > -Jim
> >
> > > -----Original Message-----
> > > From: George Harley1 [mailto:GHARLEY@xxxxxxxxxx]
> > > Sent: Friday, May 09, 2003 9:05 AM
> > > To: aspectj-dev@xxxxxxxxxxx
> > > Subject: [aspectj-dev] rc2 **Preview Build #4** and new failures (!)
> > >
> > > Hi,
> > >
> > > Another build of 1.1rc2 has been made and the output aspectj-
> 1.1rc2.jar
> > > placed in org.aspectj/releases/aspectj-1.1rc2 on CVS. The test results
> > > files have also been updated with the outcome of the unit tests and
> > > Harness
> > > tests on both 131 and 141 JDKs. An IBM 131 JDK and a Sun 141 JDK were
> used
> > > for the tests.
> > >
> > > The build was carried out on a 1.3.1 JDK and the CVS extract was done
> > > today
> > > at about 10:40am BST.
> > >
> > > JUnit Tests
> > > -----------------
> > > 100% pass rate running on Sun 1.4.1, Sun 1.3.1 and IBM 1.3.1
> > > Results for the Sun 1.4.1 run in junitModules.report.JDK14.zip in the
> > > releases directory. Results for IBM 1.3.1 in junitModules.report.zip
> also
> > > in the releases directory.
> > >
> > > Harness tests
> > > ---------------------
> > > OK, things start getting a bit interesting here. With the source
> extract
> > > taken for this build all of the harness tests against ajcTests.xml had
> > > precisely 2 test failures. The failures were the same across JVMs and
> were
> > > unaffected by the emacssym arg. In both cases a dependency could not
> be
> > > resolved because the output dir of the compile was not on the
> CLASSPATH.
> > > Here is failure #1 ....
> > >
> > > ------------ FAIL: Reading inner classes from source and bytecode (1)
> --
> > > was failing()
> > > --- result: false
> > > [ 0] ------------ PASS: CompilerRun(CompilerRun.Spec AjdtCommand(2
> > > paths))
> > > [ 0] --- result: true
> > > [ 0] --- messages
> > > [ 0] MessageHolder: (6 info)
> > > [ 0] [info 0]: info org.aspectj.ajdt.ajc.AjdtCommand@494072f8([-d,
> C:
> > > \Documents and Settings\gharley\Local
> Settings\Temp\SandboxOHbBVe\classes,
> > > -classpath, ..\lib\test\aspectjrt.jar;..\lib\test\testing-client.jar,
> C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\C.jav
> > > a,
> > >
> > > C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\D.jav
> > > a])
> > >
> > >
> > > [ 0] [info 1]: info directory classpath entry does not exist: C:
> > > \ibm-sdk-131-sr4\jre\classes
> > > [ 0] [info 2]: info compiling C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\C.jav
> > > a
> > >
> > >
> > > [ 0] [info 3]: info compiling C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\D.jav
> > > a
> > >
> > >
> > > [ 0] [info 4]: info weaving
> > > [ 0] [info 5]: info might need to weave
> [UnwovenClassFile(C:\Documents
> > > and Settings\gharley\Local
> > > Settings\Temp\SandboxOHbBVe\classes\C$I$J$K.class, C$I$J$K),
> > > UnwovenClassFile(C:\Documents and Settings\gharley\Local
> > > Settings\Temp\SandboxOHbBVe\classes\C.class, C), UnwovenClassFile(C:
> > > \Documents and Settings\gharley\Local
> > > Settings\Temp\SandboxOHbBVe\classes\C$I.class, C$I),
> UnwovenClassFile(C:
> > > \Documents and Settings\gharley\Local
> > > Settings\Temp\SandboxOHbBVe\classes\C$I$J.class, C$I$J),
> > > UnwovenClassFile(C:\Documents and Settings\gharley\Local
> > > Settings\Temp\SandboxOHbBVe\classes\D.class, D)](world=false)
> > > [ 1] ------------ FAIL: CompilerRun(CompilerRun.Spec AjdtCommand(2
> > > paths))
> > > [ 1] --- result: false
> > > [ 1] --- messages
> > > [ 1] MessageHolder: (7 info) (2 error) (2 fail)
> > > [ 1] [info 0]: info org.aspectj.ajdt.ajc.AjdtCommand@23bc72fa([-d,
> C:
> > > \Documents and Settings\gharley\Local
> Settings\Temp\SandboxOHbBVe\classes,
> > > -classpath, ..\lib\test\aspectjrt.jar;..\lib\test\testing-client.jar,
> C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\Main.
> > > java,
> > >
> > > C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\C.jav
> > > a])
> > >
> > >
> > > [ 1] [info 1]: info directory classpath entry does not exist: C:
> > > \ibm-sdk-131-sr4\jre\classes
> > > [ 1] [info 2]: info compiling C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\Main.
> > > java
> > >
> > >
> > > [ 1] [info 3]: info compiling C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\C.jav
> > > a
> > >
> > >
> > > [ 1] [info 4]: info CompilerRun.Spec AjdtCommand(2 paths)
> > > [ 1] [info 5]: info [-d, C:\Documents and Settings\gharley\Local
> > > Settings\Temp\SandboxOHbBVe\classes, -classpath, ..
> > > \lib\test\aspectjrt.jar;..\lib\test\testing-client.jar, C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\Main.
> > > java,
> > >
> > > C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\C.jav
> > > a]
> > >
> > >
> > > [ 1] [info 6]: info TestSetup(org.aspectj.ajdt.ajc.AjdtCommand )
> > > [ 1] [error 0]: error D cannot be resolved or is not a type at C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\Main.
> > > java:97:0
> > > [ 1] [error 1]: error D cannot be resolved or is not a type at C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\Main.
> > > java:97:0
> > > [ 1] [fail 0]: fail unexpected error: error D cannot be resolved
> or is
> > > not a type at C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\Main.
> > > java:97:0
> > >
> > >
> > > [ 1] [fail 1]: fail unexpected error: error D cannot be resolved
> or is
> > > not a type at C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\innersFromSourceAndBytecode\Main.
> > > java:97:0
> > >
> > >
> > > FAIL Reading inner classes from source and bytecode (1) -- was
> failing()
> > >
> > > ... and here is failure #2 ....
> > >
> > > ------------ FAIL: interfaces with non-explicitly static inner
> classes()
> > > --- result: false
> > > [ 0] ------------ PASS: CompilerRun(CompilerRun.Spec AjdtCommand(1
> > > paths))
> > > [ 0] --- result: true
> > > [ 0] --- messages
> > > [ 0] MessageHolder: (5 info)
> > > [ 0] [info 0]: info org.aspectj.ajdt.ajc.AjdtCommand@7fb1f2e7([-d,
> C:
> > > \Documents and Settings\gharley\Local
> Settings\Temp\SandboxZDWzJ2\classes,
> > > -classpath, ..\lib\test\aspectjrt.jar;..\lib\test\testing-client.jar,
> C:
> > > \eclipse\eclipse-2.1-
> > >
> Release\eclipse\workspace\tests\pureJava\InterfaceAndInnerHelper.java])
> > >
> > >
> > > [ 0] [info 1]: info directory classpath entry does not exist: C:
> > > \ibm-sdk-131-sr4\jre\classes
> > > [ 0] [info 2]: info compiling C:
> > > \eclipse\eclipse-2.1-
> > > Release\eclipse\workspace\tests\pureJava\InterfaceAndInnerHelper.java
> > >
> > >
> > > [ 0] [info 3]: info weaving
> > > [ 0] [info 4]: info might need to weave
> [UnwovenClassFile(C:\Documents
> > > and Settings\gharley\Local
> > > Settings\Temp\SandboxZDWzJ2\classes\InterfaceAndInnerHelper.class,
> > > InterfaceAndInnerHelper), UnwovenClassFile(C:\Documents and
> > > Settings\gharley\Local
> > > Settings\Temp\SandboxZDWzJ2\classes\InterfaceAndInnerHelper$C.class,
> > > InterfaceAndInnerHelper$C)](world=false)
> > > [ 1] ------------ FAIL: CompilerRun(CompilerRun.Spec AjdtCommand(1
> > > paths))
> > > [ 1] --- result: false
> > > [ 1] --- messages
> > > [ 1] MessageHolder: (6 info) (2 error) (2 fail)
> > > [ 1] [info 0]: info org.aspectj.ajdt.ajc.AjdtCommand@5f2532e6([-d,
> C:
> > > \Documents and Settings\gharley\Local
> Settings\Temp\SandboxZDWzJ2\classes,
> > > -classpath, ..\lib\test\aspectjrt.jar;..\lib\test\testing-client.jar,
> C:
> > > \eclipse\eclipse-2.1-
> > > Release\eclipse\workspace\tests\pureJava\InterfaceAndInner.java])
> > >
> > >
> > > [ 1] [info 1]: info directory classpath entry does not exist: C:
> > > \ibm-sdk-131-sr4\jre\classes
> > > [ 1] [info 2]: info compiling C:
> > > \eclipse\eclipse-2.1-
> > > Release\eclipse\workspace\tests\pureJava\InterfaceAndInner.java
> > >
> > >
> > > [ 1] [info 3]: info CompilerRun.Spec AjdtCommand(1 paths)
> > > [ 1] [info 4]: info [-d, C:\Documents and Settings\gharley\Local
> > > Settings\Temp\SandboxZDWzJ2\classes, -classpath, ..
> > > \lib\test\aspectjrt.jar;..\lib\test\testing-client.jar, C:
> > > \eclipse\eclipse-2.1-
> > > Release\eclipse\workspace\tests\pureJava\InterfaceAndInner.java]
> > >
> > >
> > > [ 1] [info 5]: info TestSetup(org.aspectj.ajdt.ajc.AjdtCommand )
> > > [ 1] [error 0]: error InterfaceAndInnerHelper cannot be resolved or
> is
> > > not a type at C:
> > > \eclipse\eclipse-2.1-
> > > Release\eclipse\workspace\tests\pureJava\InterfaceAndInner.java:7:0
> > > [ 1] [error 1]: error InterfaceAndInnerHelper cannot be resolved or
> is
> > > not a type at C:
> > > \eclipse\eclipse-2.1-
> > > Release\eclipse\workspace\tests\pureJava\InterfaceAndInner.java:8:0
> > > [ 1] [fail 0]: fail unexpected error: error
> InterfaceAndInnerHelper
> > > cannot be resolved or is not a type at C:
> > > \eclipse\eclipse-2.1-
> > > Release\eclipse\workspace\tests\pureJava\InterfaceAndInner.java:7:0
> > >
> > >
> > > [ 1] [fail 1]: fail unexpected error: error
> InterfaceAndInnerHelper
> > > cannot be resolved or is not a type at C:
> > > \eclipse\eclipse-2.1-
> > > Release\eclipse\workspace\tests\pureJava\InterfaceAndInner.java:8:0
> > >
> > >
> > > FAIL interfaces with non-explicitly static inner classes()
> > >
> > > It looks like both of the above are related to Wes' changes last
> > > night/this
> > > morning (see his earlier posting entitled "incremental not resolving
> > > unmodified types") which took out the code that added the output
> directory
> > > to the classpath. Taking to heart the advice from the end of his note
> ("if
> > > you are getting problems during incremental
> > > compilation with types in unmodified files not being resolved, you can
> try
> > > working around them for now by adding the output directory to the
> > > classpath")I made a change to the test harness to add the output dir
> back
> > > into the CLASSPATH and all the tests passed once again. Here is the
> patch
> > > to org.aspectj.testing.harness.bridge.CompilerRun ....
> > >
> > > ------------------------------8<----------SNIP SNIP-----------8
> > > <------------------------
> > >
> > > Index: CompilerRun.java
> > > ===================================================================
> > > RCS file:
> > >
> /home/technology/org.aspectj/modules/testing/src/org/aspectj/testing/harne
> > > ss/bridge/CompilerRun.java,v
> > > retrieving revision 1.7
> > > diff -u -r1.7 CompilerRun.java
> > > --- CompilerRun.java 9 May 2003 07:14:19 -0000 1.7
> > > +++ CompilerRun.java 9 May 2003 15:54:13 -0000
> > > @@ -262,9 +262,10 @@
> > >
> > > // save classpath and aspectpath in sandbox for this and
> other
> > > clients
> > > final boolean checkReadable = true; // hmm - third
> validation?
> > > - File[] cp = new File[2 + classFiles.length];
> > > + File[] cp = new File[3 + classFiles.length];
> > > System.arraycopy(classFiles, 0, cp, 0, classFiles.length);
> > > int index = classFiles.length;
> > > + cp[index++] = sandbox.classesDir;
> > > cp[index++] = Globals.F_aspectjrt_jar;
> > > cp[index++] = Globals.F_testingclient_jar;
> > > sandbox.setClasspath(cp, checkReadable, this);
> > >
> > > ------------------------------>8----------SNIP
> > > SNIP----------->8------------------------
> > >
> > > NOTE : the above has not been committed to CVS.
> > >
> > > So now the latest 1.1rc2 build passes all of its tests (including
> > > installation and post-install tests) but with a tweak to the harness
> > > testing that masks the classpath problem highlighted by Wes. What is
> the
> > > consensus on the validity of these tests and hence of this build as
> our
> > > rc2
> > > ?
> > >
> > >
> > > Best regards,
> > > George
> > > ________________________________________
> > > George C. Harley
> > > IBM e-Business Integration Technologies
> > > MP 146, Hursley Park, Winchester, Hants, UK
> > > External : +44 (0)1962 819549 / Internal : 7-249549
> > > Fax : +44 (0)1962 818999
> > > Email : gharley@xxxxxxxxxx
> > > Web : http://ibm.com/java
> > >
> > >
> > > _______________________________________________
> > > aspectj-dev mailing list
> > > aspectj-dev@xxxxxxxxxxx
> > > http://dev.eclipse.org/mailman/listinfo/aspectj-dev
> > _______________________________________________
> > aspectj-dev mailing list
> > aspectj-dev@xxxxxxxxxxx
> > http://dev.eclipse.org/mailman/listinfo/aspectj-dev
>
> _______________________________________________
> aspectj-dev mailing list
> aspectj-dev@xxxxxxxxxxx
> http://dev.eclipse.org/mailman/listinfo/aspectj-dev
