Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call the "activated" method of the default tab in a Launch Config Dialog #860

Conversation

fedejeanne
Copy link
Contributor

@fedejeanne fedejeanne commented Nov 16, 2023

This PR adds a fix (#859) and also a new test bundle that contains:

FWIW I followed these instructions to add the new test bundle (If they are outdated, please let me know) and I also added the new bundle as a <module> in the pom.xml.

Fixes: #859
Contributes to: eclipse-pde/eclipse.pde#674

@fedejeanne fedejeanne force-pushed the bugs/activate_first_tab_in_LaunchConfigurationTabGroupViewer branch from 3aeb7d9 to 3717efa Compare November 16, 2023 09:19
@HeikoKlare
Copy link
Contributor

Please consider that there already exist debug.ui tests in the org.eclipse.debug.tests` project, such as this.

So either new tests should be added there as well or creating a org.eclipse.debug.ui.tets project should be a preparatory refactoring for this PR, in which existing tests are moved to that project.

Copy link
Contributor

github-actions bot commented Nov 16, 2023

Test Results

     591 files  +  18       591 suites  +18   1h 7m 54s ⏱️ +27s
  3 843 tests +    5    3 838 ✔️ +    6    5 💤 ±0  0  - 1 
12 135 runs  +162  12 099 ✔️ +163  36 💤 ±0  0  - 1 

Results for commit 2fe8228. ± Comparison against base commit e4502ef.

♻️ This comment has been updated with latest results.

@fedejeanne fedejeanne force-pushed the bugs/activate_first_tab_in_LaunchConfigurationTabGroupViewer branch from 3717efa to 80acc22 Compare November 16, 2023 10:04
@fedejeanne
Copy link
Contributor Author

@iloveeclipse since you created org.eclipse.debug.tests.ui.VariableValueEditorManagerTests (see previous comment) do you think I should move my new test class right alongside yours or should VariableValueEditorManagerTests be moved to the new test plugin? For the last option, I'd rather do it in a separate PR and use this one as preparatory work.

@fedejeanne
Copy link
Contributor Author

Also, I'm puzzled about the errors. I see these 3 errors in the Verify Windows step:

1: Problems in org.eclipse.help.base

Error:  Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.4:verify (verify) on project org.eclipse.help.base: There are API errors:
Error:  src/org/eclipse/help/base/AbstractHelpDisplay.java:24 The type org.eclipse.help.base.AbstractHelpDisplay has been removed from org.eclipse.help.base_4.4.200
...
Error:  src/org/eclipse/help/standalone/Infocenter.java:29 The type org.eclipse.help.standalone.Infocenter has been removed from org.eclipse.help.base_4.4.200
Error:  META-INF/MANIFEST.MF:5 The major version should be incremented in version 4.4.200, since API breakage occurred since version 4.4.100
Error:  -> [Help 1]

It seems to be unrelated to this PR and it kind of looks like a previous failure which could be caused by Tycho (see comment from Andrey).

2: Problems in org.eclipse.debug.ui.tests (My new bundle)

Error:  Failed to execute goal org.eclipse.tycho:tycho-surefire-plugin:4.0.4:test (default-test) on project org.eclipse.debug.ui.tests: An unexpected error occurred while launching the test runtime (process returned error code 13 (HRESULT Code 0xD, check for example https://www.hresult.info/ for further details)). The process logfile D:\a\eclipse.platform\eclipse.platform\debug\org.eclipse.debug.ui.tests\target\work\data\.metadata\.log might contain further details. Command-line used to launch the sub-process was C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\17.0.9-9.1\x64\bin\java.exe -Dosgi.noShutdown=false -Dosgi.os=win32 -Dosgi.ws=win32 -Dosgi.arch=x86_64 --add-modules=ALL-SYSTEM -Dosgi.clean=true -ea -jar C:\Users\runneradmin\.m2\repository\p2\osgi\bundle\org.eclipse.equinox.launcher\1.6.600.v20231106-1826\org.eclipse.equinox.launcher-1.6.600.v20231106-1826.jar -data D:\a\eclipse.platform\eclipse.platform\debug\org.eclipse.debug.ui.tests\target\work\data -install D:\a\eclipse.platform\eclipse.platform\debug\org.eclipse.debug.ui.tests\target\work -configuration D:\a\eclipse.platform\eclipse.platform\debug\org.eclipse.debug.ui.tests\target\work\configuration -application org.eclipse.tycho.surefire.osgibooter.headlesstest -testproperties D:\a\eclipse.platform\eclipse.platform\debug\org.eclipse.debug.ui.tests\target\surefire.properties in working directory D:\a\eclipse.platform\eclipse.platform\debug\org.eclipse.debug.ui.tests -> [Help 1]

It says:

The process logfile D:\a\eclipse.platform\eclipse.platform\debug\org.eclipse.debug.ui.tests\target\work\data.metadata.log might contain further details

Any hint on what the problem might be?
Were can I find the aforementioned log file?

3: Problems in org.eclipse.debug.tests

Documented in #861

@fedejeanne fedejeanne force-pushed the bugs/activate_first_tab_in_LaunchConfigurationTabGroupViewer branch from 80acc22 to 1411bd2 Compare November 16, 2023 10:50
@fedejeanne fedejeanne marked this pull request as ready for review November 16, 2023 10:51
@fedejeanne
Copy link
Contributor Author

@akurtakov are these instructions up to date: https://wiki.eclipse.org/Platform-releng-faq#What_needs_to_be_done_to_add_a_new_test_bundle? I noticed you worked on them back in February 2022

image

@iloveeclipse
Copy link
Member

do you think I should move my new test class right alongside yours

Yes, please do not create new bundle. Historically org.eclipse.debug.tests contain both core/ui tests, you can see there are console tests, memory view tests etc.

Problems in org.eclipse.help.base

This is known tycho issue eclipse-tycho/tycho#3019 where tycho now insists that build.properties contain an extra entry for bin directory (previously this wasn't needed). The workaround is to add that entry in the build.properties of affected bundle(s).

@akurtakov
Copy link
Member

@akurtakov are these instructions up to date: https://wiki.eclipse.org/Platform-releng-faq#What_needs_to_be_done_to_add_a_new_test_bundle? I noticed you worked on them back in February 2022

They used to be back then. I can't remember anything big that changed so it should be correct.

@HeikoKlare
Copy link
Contributor

Any hint on what the problem might be?
Were can I find the aforementioned log file?

You can find the log file in the according Jenkins run. The log states that there are warnings in the added code, which are treated as errors:

error: warnings found and -failOnWarning specified

@fedejeanne fedejeanne force-pushed the bugs/activate_first_tab_in_LaunchConfigurationTabGroupViewer branch from 1411bd2 to 92b20bb Compare November 16, 2023 12:37
@iloveeclipse iloveeclipse force-pushed the bugs/activate_first_tab_in_LaunchConfigurationTabGroupViewer branch 2 times, most recently from a2cdf99 to 4e96983 Compare November 16, 2023 12:49
@iloveeclipse
Copy link
Member

@fedejeanne : I've pushed fixed tests, please check.

@HeikoKlare
Copy link
Contributor

@fedejeanne @iloveeclipse MANIFEST.MF and pom.xml need to be "reverted".
I am currently blocked, so cannot adapt the commit.

@fedejeanne fedejeanne force-pushed the bugs/activate_first_tab_in_LaunchConfigurationTabGroupViewer branch from 4e96983 to a3f81d6 Compare November 16, 2023 13:05
@fedejeanne
Copy link
Contributor Author

fedejeanne commented Nov 16, 2023

@fedejeanne @iloveeclipse MANIFEST.MF and pom.xml need to be "reverted". I am currently blocked, so cannot adapt the commit.

Done

@fedejeanne : I've pushed fixed tests, please check.

I haven't seen any changes in the tests, where did push exactly? FWIW the tests run locally and they seem to run in GH too since all checks have passed. This PR looks ready to be merged, wouldn't you agree?

@iloveeclipse
Copy link
Member

I haven't seen any changes in the tests, where did push exactly?

On your github branch. Unfortunately you discarded them.

This PR looks ready to be merged, wouldn't you agree?

No, see my comment above:

known tycho issue eclipse-tycho/tycho#3019 where tycho now insists that build.properties contain an extra entry for bin directory (previously this wasn't needed). The workaround is to add that entry in the build.properties of affected bundle(s).

@HeikoKlare
Copy link
Contributor

I haven't seen any changes in the tests, where did push exactly?

On your github branch. Unfortunately you discarded them.

@fedejeanne The commit is still there: 4e96983

@fedejeanne
Copy link
Contributor Author

I haven't seen any changes in the tests, where did push exactly?

On your github branch. Unfortunately you discarded them.

Oh, sorry, I must have force-pushed before I got your message. Care to push again or are the problems gone/fixed now?

This PR looks ready to be merged, wouldn't you agree?

No, see my comment above:

known tycho issue eclipse-tycho/tycho#3019 where tycho now insists that build.properties contain an extra entry for bin directory (previously this wasn't needed). The workaround is to add that entry in the build.properties of affected bundle(s).

I added another PR for that one since I think it's unrelated to this PR --> #862. It's looking good, thank you for the hint!

@HeikoKlare
Copy link
Contributor

Oh, sorry, I must have force-pushed before I got your message. Care to push again or are the problems gone/fixed now?

You can just compare with 4e96983 to see the changes and integrate them again.

@fedejeanne
Copy link
Contributor Author

Oh, sorry, I must have force-pushed before I got your message. Care to push again or are the problems gone/fixed now?

You can just compare with 4e96983 to see the changes and integrate them again.

Thanks! I just compared them and saw that Andrey added the test class to the suite. I just happened to add those same changes in a3f81d6

Also add a new test bundle that contains the proper regression test.

Bundle: org.eclipse.debug.ui.tests
Class: LaunchConfigurationTabGroupViewerTest

This class also contains a regression test for
eclipse-platform#766

Fixes: eclipse-platform#859
@vogella
Copy link
Contributor

vogella commented Nov 29, 2023

Is this one ready, if yes please merge

@fedejeanne fedejeanne merged commit c910372 into eclipse-platform:master Nov 29, 2023
16 checks passed
@fedejeanne fedejeanne deleted the bugs/activate_first_tab_in_LaunchConfigurationTabGroupViewer branch November 29, 2023 09:49
@fedejeanne
Copy link
Contributor Author

This PR unblocked eclipse-pde/eclipse.pde#946

fyi @HannesWell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The first tab is not properly activated when opening the "Run configurations" dialog
5 participants