Refresh option not available for some resources that are not closed p… #2601
Conversation
|
@mickaelistria can you review this? #2003 (comment) |
|
The failing ProgressContantsTest.testKeepOneProperty is unrelated see #370 |
|
Code looks good and safe enough. However we didn't get any build completing from https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/PR-2601/ , and I wouldn't feel comfortable merging until we have 1 build running entirely. |
|
For the record: Build failed with OutOfMemoryError, i have not seen that elsewhere. @DaveCarpeneto can you help investigate the reason for OOME? May be its even related to this PR? |
|
The OutOfMemory seems to be already discussed in #2432 |
|
The CI logfile contains messages like does it relate to this PR? |
|
i don't get if the added tests are useful: Only one of them fails when the PR is reverted and has an error message that is not understandable to me: |
The one test that fails relates directly to the code change. The other 4 handle other situations where eclipse behaves as expected today. I added those 4 tests in the hope that future changes do not introduce a regression bug, like was done in the code added for #1438 .
My hope was that the error message was enough that if this test failed that people investigating would have enough information to see what went wrong. In this case RefreshAction was not included, where the test was expecting to be included. If you feel that further elaboration is needed for a unit test failure message please let me know & I will submit another change to cover this. |
No. I see this in the stdout when I grab pretty much any recent build & run these tests (which succeed). The NotHandledException is in fact what was expected by the test. These lines come from things like the following in org.eclipse.ui.tests.commands.CommandExecutionTest : ... which were added as part of the tests in 2012. These lines have been reported in builds for quite some time :-) |
|
Just checking to see if there's anything else of concern on this pull request. To recap responses to the previously-raised concerns:
Thanks for all the feedback given thus far. Please do let me know if there's anything else I can do to get this into product, ideally for 2025-03 (the 1-year anniversary of the regression bug's introduction) |
|
I think this error comes from your change: Of course in Javadoc it should be |
Ouch. Yes. Fixed. |
|
please rebase on master |
|
Sorry we have so many "rules". But it helps to keep things clean: https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#creating-a-pull-request E.g., we really would prefer that there is a single commit which generally involves amending and force pushing and you make changes (with the exception of version increments). So rebasing on master and squashing into a single commit would be good... |
|
The failure looks unrelated to your changes. I was going to rebase the branch, but it just now got rebased with a merge commit creating a 3rd commit; there's actually two choice on that green button and the first default one is not good. We definitely block merge commits. Anyway, let's see a green build first.... |
|
The one failure is intimately familiar and goes away if we re-run it until it doesn't randomly fail:
The other is because we don't allow merge commits:
If we had this as a single commit based on master, I think all would be good, at least from a build point of view. I don't see that anyone has actually reviewed the code yet. 😞 |
This was done in previously submitted pull requests. See comments in: #2542#2539... the problem is that I'm not used to github & am making a mess of things as I go. As such when I'm told to put things into a single squashed commit it's less risky for me to just close the existing PR & open a new one. Apologies for the confusion this causes. DOing the best I can in a strange (to me) environment.
As per the above: once people are comfy with everything in this PR I'll create another one with a single commit in it. |
|
I'm not sure if you are using EGit in the IDE. What I do is rename my PR branch foo to foo-old. Then I check out master. Make sure I pull master. Then I create a new branch foo (name I used before for the PR) which is based on the update-to-date master. I make sure all my changes are in the workspace, I make a new commit onto the foo branch and I force push foo to my fork. This updates the existing PR that was created from foo. This will make everyone happier. If you mess it up, no one will die. |
DaveCarpeneto
force-pushed
the
issue2538b
branch
from
cc8ef59 to
e9e5521
Compare
The list of happier people includes me :-) Thank you for these instructions @merks - this matches what I did, minus the branch rename step. I think I did this correctly? Please let me know if there's anything incorrect / needing changed from my end. |
|
Yes, there is now a single commit on this existing PR. 😁 |
|
LGTM, i have restarted linux verify. once its green we can merge. |
|
I guess we spin until the too familiar error is gone:
|
|
I feel stupid, but I see now there is one test failure, rather an an API baseline disposed failure, but for the life of me I cannot find a link to navigate me to information about which test failed. Part of me says, hey, the jenkins build was good and it runs on Linux too, so what the heck is wrong with the github action for Linux. I expect you have more experience "reading the tea leaves" for this kind of thing. I would greatly appreciate if you explained how I locate the details for a case like this!! |
|
the relevant jenkins job https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/PR-2601/8/ is green so this seem to be an error, that that result is not forwared to github test results. lets rebase to get a new build. |
…rojects eclipse-platform#2538 Change made since the fix to eclipse-platform#1438 unintentionally removed the "refresh" contextual menu for resources that are not projects. With this change the "refresh" contextual menu is shown if ANY navigator selection is either (A) an open project, or (B) a non-project resource. Put another way: the 'refresh' item is NOT shown if ALL selections are closed projects. Fixes eclipse-platform#2538
|
It just got 1 degree warmer in Switzerland and I see less snow on Mount Pilatus. On the plus side, I feel less stupid. 👿 |
|
The build failed again. Once the build is green this PR can be merged. Even though it is not contributors failure i can not spend my time in fixing the build. Please consider to help on improving the build process. |
|
I'll kick it a few times, or until I scream, whichever comes first. |
|
For future reference, if you ever want to update the branch, use the second (rebase) choice which is not the default so never just push the button:
Don't do it now or all the builds will start from scratch again! |
|
I will run GHA for Linux one last time, and then I will merge this regardless of the outcome. |
|
Failure is known #2603 and seem unrelated so Merging now should imho be fine |
|
Thank you for your contribution and your patience. 🏆 |
|
@merks thanks for your patience :-) |
Don't mistake persistence and perseverance for patience. They only look similar from the outside. 😱 😜 |
6 participants
…rojects #2538
Change made since the fix to #1438 unintentionally removed the "refresh" contextual menu for resources that are not projects.
With this change the "refresh" contextual menu is shown if ANY navigator selection is either (A) an open project, or (B) a non-project resource. Put another way: the 'refresh' item is NOT shown if ALL selections are closed projects.
Fixes #2538