search

eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

fix ContextInformationTest #35 #212

Closed jukzi closed 1 year ago

jukzi commented 1 year ago

Identify correct shell instead of taking random one.

vogella commented 1 year ago

Thanks @jukzi , changes looks good in my browser. If this builds fine, you have my go as PMC to merge this if you want.

jukzi commented 1 year ago

Maybe some linux user can investigate why ContextInformationTest.testContextInfo_hide_focusOut() does not dispose the shell? @HeikoKlare? I disabled that test for linux again, but it would be better to find and fix the reason.

vogella commented 1 year ago

Why are the GH verification actions not running anymore? @laeubi I think you moved this repo to use the unified verification workflow. Any idea why the GH action are not running anymore?

laeubi commented 1 year ago

Platform text has not github actions for compilations: https://github.com/eclipse-platform/eclipse.platform.text/tree/master/.github/workflows

As it was palnned to merge the repo into platform.ui I'm not sure if it worth the effort to set this up?!

jukzi commented 1 year ago

Also fixing CompletionTest as failing in https://download.eclipse.org/eclipse/downloads/drops4/I20230525-0350/testresults/html/org.eclipse.ui.genericeditor.tests_ep428I-unit-win32-java17_win32.win32.x86_64_17.html

No new shell found expected:<1> but was:<2>

java.lang.AssertionError: No new shell found expected:<1> but was:<2>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.eclipse.ui.genericeditor.tests.CompletionTest.findNewShell(CompletionTest.java:206)
at org.eclipse.ui.genericeditor.tests.CompletionTest.testCompletionFreeze_bug521484(CompletionTest.java:215)
vogella commented 1 year ago

@jukzi please increase version if you want to have this in ASAP.

09:59:11.293 [ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.0-SNAPSHOT:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.ui.genericeditor.tests: Only qualifier changed for (org.eclipse.ui.genericeditor.tests/1.3.0.v20230526-0951). Expected to have bigger x.y.z than what is available in baseline (1.3.0.v20230222-1453) -> [Help 1]

Change is now PMC approved

mickaelistria commented 1 year ago

As a reminder, changes in tests do not require any approval during RCs. As per the plan for RC2 "Ongoing changes to documentation, tests or examples do not require approval. "

jukzi commented 1 year ago

As a reminder, changes in tests do not require any approval during RCs. As per the plan for RC2 "Ongoing changes to documentation, tests or examples do not require approval. "

Thanks for the reminder. Reviews an approvals are still very welcome and encouraging.

HeikoKlare commented 1 year ago

Maybe some linux user can investigate why ContextInformationTest.testContextInfo_hide_focusOut() does not dispose the shell? @HeikoKlare? I disabled that test for linux again, but it would be better to find and fix the reason.

Actually, I usually use Windows, but maybe I find some time to investigate this on a Linux system. @jukzi Should we have an issue to document that this test should be reenabled for Linux (any maybe also for MacOS)? Then also others could see that there is an issue that can be addressed. Maybe, it could also serve as a "good first issue"?

jukzi commented 1 year ago

Actually, I usually use Windows, but maybe I find some time to investigate this on a Linux system. @jukzi Should we have an issue to document that this test should be reenabled for Linux (any maybe also for MacOS)? Then also others could see that there is an issue that can be addressed. Maybe, it could also serve as a "good first issue"?

I think the assume errors indicating a disabled test put enough information. Maybe a combined issue for all disabled tests as "good first issue"?