Community
Participate
Working Groups
Bug to track porting of the test bundles in platform.text to JUnit 4
New Gerrit change created: https://git.eclipse.org/r/65788
Gerrit change https://git.eclipse.org/r/65788 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=4e7aa5c62f126bb1ab1f6a4a527176bb2debd450
New Gerrit change created: https://git.eclipse.org/r/65797
Gerrit change https://git.eclipse.org/r/65797 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=b0506297da6a094d95545ef5aa683ea71091420c
New Gerrit change created: https://git.eclipse.org/r/65808
Gerrit change https://git.eclipse.org/r/65808 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=fec66960072003e3f884ea68ed4f19ea82dce76e
New Gerrit change created: https://git.eclipse.org/r/65822
Gerrit change https://git.eclipse.org/r/65822 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=805de570454ef7bb1d956a147cdc9bc6b8faa0b9
New Gerrit change created: https://git.eclipse.org/r/65828
Gerrit change https://git.eclipse.org/r/65828 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=79782e1b3df69cff447cb2c40093172cd769117b
New Gerrit change created: https://git.eclipse.org/r/65830
Gerrit change https://git.eclipse.org/r/65830 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=86d7d066a8cdcbeaa1a8eee30ef8b529893652e3
Due to patches being big they were split in individual projects in this order: org.eclipse.core.filebuffers.tests org.eclipse.jface.text.tests org.eclipse.search.tests org.eclipse.text.tests org.eclipse.ui.editors.tests org.eclipse.ui.workbench.texteditor.tests Port is complete.
(In reply to Sopot Cela from comment #13) > Port is complete. Not really, see bug 487180 and http://download.eclipse.org/eclipse/downloads/drops4/N20160203-2000/compilelogs/plugins/org.eclipse.jdt.text.tests_3.12.0.qualifier/@dot.html
This used to be an (IMO unnecessary) enhancement request, but now it's a major regression. Whenever you touch code, you're supposed to leave it in a better state than before. These changes brought zero advantage, lots of new bugs, and stole me hours of review time. * Why did you remove the 'final' modifier on org.eclipse.jface.text.tests.AbstractPairMatcherTest.fCaretEitherSideOfBracket ? Please revert. It's useful to know that a field is not modified after initialization. In contrast to 'final' on local variables, code bloat is not an issue for fields. And in the future, don't include bad manual changes in the same commit as machine-generated changes. * Fix the wrong MANIFEST.MFs: > Require-Bundle: org.junit;bundle-version="3.8.2" needs to require JUnit 4. * FileCharSequenceTests run into an endless recursion. The test was already broken before, and it was/is not included in the AllSearchTests. Nevertheless, you should not add more bugs in tests that didn't need a change. (I wanted to release a fix a while ago, but bad commits such a this one kept me too busy.) * SearchResultPageTest had two tests that were accidentally disabled. They were both green, so adding @Ignore was wrong. * SearchResultPageTest#testTableNavigation() is red if you enable it in master, but that's not because the test is broken, but because you wrongly removed the JUnitSourceSetup. * Removing JUnitSourceSetup was wrong and turned some suites (e.g. LineAnnotationManagerTest) into no-ops. Inlining its functionality violates basic software engineering principles. You have to restore the class, make sure the implementation and constants are not duplicated, and then make sure the JUnit4 tests use the single implementation in the same way as before. * Are you sure that removing OrderedTestSuite from FindReplaceDialogTest was OK? I don't have time to investigate, but testInitialButtonState() looks like it needs to be the first one, and probably only doesn't fail by chance right now.
(In reply to Markus Keller from comment #15) > * FileCharSequenceTests run into an endless recursion. You have to re-check all other places where you touched assert calls. Maybe you used "Add Import" and ran into bug 487247.
(In reply to Markus Keller from comment #16) > (In reply to Markus Keller from comment #15) > > * FileCharSequenceTests run into an endless recursion. > > You have to re-check all other places where you touched assert calls. Maybe > you used "Add Import" and ran into bug 487247. Since this is almost everywhere. I'll revert everything and get back to this once bug 487247 is fixed. As for the previous comment: *FileCharSequenceTests - I did _not_ enable it and I did _not_ make any change in code other than the porting changes. * SearchResultPageTest - I left the disabled tests as disabled. Enabling tests is out of the scope of this bug. Also testTableNavigation() is green for me when I run the suite. * FindReplaceDialogTest- I tested this multiple times to check whether it was the order and it worked fine.
(In reply to Sopot Cela from comment #17) > Since this is almost everywhere. I'll revert everything and get back to this > once bug 487247 is fixed. No, please keep it and start fixing right now. I'm done with the review and I've already released the adoption fix in jdt.text tests (bug 487180). Remaining problems: FindReplaceDialogTest#takeScreenshot() and ScreenshotTest tests don't identify test names any more. You probably have to use an @Rule TestName. > * Removing JUnitSourceSetup was wrong and turned some suites (e.g. > LineAnnotationManagerTest) into no-ops. AnnotationManagerTest is probably another victim of that. testBogusAnnotation() fails when you run this suite alone.
> *FileCharSequenceTests - I did _not_ enable it and I did _not_ make any > change in code other than the porting changes. Yes, but the porting changes left the class in a worse state than before. > * SearchResultPageTest - [..] Also testTableNavigation() is green > for me when I run the suite. It's green when you run AllFileSearchTests, but red wehn you only run SearchResultPageTest. > * FindReplaceDialogTest- I tested this multiple times to check whether it > was the order and it worked fine. OK, thanks.
(In reply to Markus Keller from comment #15) > You have to restore the class, make > sure the implementation and constants are not duplicated, and then make sure > the JUnit4 tests use the single implementation in the same way as before. > Not sure what you mean here. The JUnitSourceSetup is a junit extension which is compatible only with 3.x style Tests. I don't see how this can be used as-is in a JUnit 4 test. The closest thing to it is @BeforeClass and @AfterClass static methods in test classes which would have to do what setUp and tearDown in JUniteSourceSetup do.
> > * Removing JUnitSourceSetup was wrong and turned some suites (e.g. > > LineAnnotationManagerTest) into no-ops. > > AnnotationManagerTest is probably another victim of that. > testBogusAnnotation() fails when you run this suite alone. It is related in that if I add, in AnnotationManagerTest, @BeforeClass and @AfterClass which do exactly the same as what respectively setUp and tearDown in JUnitSourceSetup do (with the exception that they have to be static in JUnit 4 case) the test works fine when you run it alone.
(In reply to Sopot Cela from comment #20) My point was that JUnitSourceSetup was a single implementation class that controlled the environment and provided getStandardProject(), so that clients could easily refer to the project and didn't have to duplicate the setup/teardown code. In AllFileSearchTests, you merged this reusable class into the test suite class. The goal is to extract it again, so that other tests can reuse it in JUnit4. Note that JUnitSourceSetups can be nested, so that: a) each individual test class could use a JUnitSourceSetup b) a TestSuite could use a JUnitSourceSetup In both cases, the JUnit project was created/deleted at most once for the whole test run. @BeforeClass/@AfterClass methods that just call the JUnitSourceSetup methods could be one way to implement this. But the right way nowadays is probably to use an @ClassRule with an ExternalResource. I haven't used that mechnaism myself, so I can't tell exactly how it works. We didn't have time to work out such things, that's why we stayed on JUnit 3.
New Gerrit change created: https://git.eclipse.org/r/66004
(In reply to Eclipse Genie from comment #23) > New Gerrit change created: https://git.eclipse.org/r/66004 This uses @ClassRule to de-inline the setup functionality into a common class, plus other improvements.
> * SearchResultPageTest had two tests that were accidentally disabled. They > were both green, so adding @Ignore was wrong. > Patch also disables these as they fail when they are run from the suite for me. Also the gerrit build fails if I enable them.
https://git.eclipse.org/r/#/c/66004/5 : Set a breakpoint in JUnitSourceSetup#setUp/tearDown() and then debug AllFileSearchTests. You can see that the project gets created/deleted multiple times. It should only be created/deleted once. Try to eliminate the CustomEnvironmentTest and rename the variable externalResource. - In contrast to JUnitSourceSetup, both new names don't tell what's going on. - As before, JUnitSourceSetup should be a one-stop solution that client code can use to a) declare that it needs this setup b) ask for getStandardProject() Why does ResultUpdaterTest need the JUnitSourceSetup now? CustomEnvironmentTest.externalResource.new ExternalResource() {...}.after() should not catch Exception. (In reply to Markus Keller from comment #18) > FindReplaceDialogTest#takeScreenshot() and ScreenshotTest tests don't > identify test names any more. You probably have to use an @Rule TestName. Works in FindReplaceDialogTest, still fails in ScreenshotTest. (In reply to Markus Keller from comment #19) > > *FileCharSequenceTests - I did _not_ enable it and I did _not_ make any > > change in code other than the porting changes. > > Yes, but the porting changes left the class in a worse state than before. Just add "Assert." to the last invocation of assertEquals(..) to fix your porting bug. Run the test with -Xmx256M to ensure testFileCharSequence3() quickly runs out of memory.
Uploaded https://git.eclipse.org/r/#/c/66004/6. Compared to 66004/5: -Used @Before and @AfterClass and made sure the setup project is created and deleted only once, whether you run a suite or single test class. -Removed any change to ResultUpdaterTest -Used @Rule TestName in ScreenhotTests -Added the Assert. in FileCharSequenceTests and observed that lowering the Xmx makes the test run out of memory. Not sure whether it makes sense to lower the Xmx for the whole test run in order to make this fail, especially since it's disabled ATM.
Gerrit change https://git.eclipse.org/r/66004 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=bf92a4cb5abe8883e4e875a2e841279558dc657b
(In reply to Sopot Cela from comment #17) > (In reply to Markus Keller from comment #15) >> * Are you sure that removing OrderedTestSuite from FindReplaceDialogTest was >> OK? I don't have time to investigate, but testInitialButtonState() looks >> like it needs to be the first one, and probably only doesn't fail by chance >> right now. > > * FindReplaceDialogTest- I tested this multiple times to check whether it > was the order and it worked fine. No, it's not fine. The only reason why it didn't fail all the time was that you made another mistake by marking tearDown() as @Before instead of as @After. This effectively made the method a no-op, since fFindReplaceDialog was always null before the actual tests ran, so FindReplaceDialog#close() was never invoked and the saving of the checkbox states was never tested. A visible outcome of this was that the Find/Replace dialog stayed open after the FindReplaceDialogTest finished. Fixed that and various other issues. See the released fix for how to format test suites, and how to properly convert JUnitSourceSetup into an ExternalResource.
Verified in I20160425-1300. Also compared test results with 4.6 M5 and verified that tests run in same order and use comparable time.
