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.

Sign up for GitHub

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

Jump to bottom

Bugs/460 test bug297635 #465

Merged
merged 1 commit into from
Jun 21, 2023
Merged

Bugs/460 test bug297635 #465

merged 1 commit into from
Jun 21, 2023

Conversation

fedejeanne
Copy link
Contributor

@fedejeanne fedejeanne commented May 26, 2023 •
edited
Loading

This fixes #460

Merge both test methods in the class TestBug297635 into 1 in order to avoid a possible workspace crash between their executions. Such a crash let (before my PR) the second test fail.

@github-actions
Copy link
Contributor

github-actions bot commented May 26, 2023 •
edited
Loading

Test Results

     30 files  +    2       30 suites  +2   49m 3s ⏱️ + 4m 57s
2 379 tests  -     1  2 378 ✔️ ±    0  1 💤 ±0  0 ❌  - 1 
7 140 runs  +141  7 136 ✔️ +142  4 💤 ±0  0 ❌  - 1 

Results for commit 0347088. ± Comparison against base commit d444af5.

This pull request removes 2 and adds 1 tests. Note that renamed tests count towards both. 
org.eclipse.core.tests.resources.session.TestBug297635 ‑ test1
org.eclipse.core.tests.resources.session.TestBug297635 ‑ test2

org.eclipse.core.tests.resources.session.TestBug297635 ‑ testBug

♻️ This comment has been updated with latest results.

@vogella
Copy link
Contributor

vogella commented May 26, 2023

@fedejeanne please remove merge commit (see https://www.vogella.com/tutorials/Git/article.html#pullrebase for a general setup).

My comments could be handled in another PR. This one looks fine to me, any other opions @jukzi ?

@jukzi
Copy link
Contributor

jukzi commented May 26, 2023

@vogella This is still draft. Please separate actual fixes from refactoring and give an explanation about the fix for easier review.

@fedejeanne fedejeanne mentioned this pull request May 26, 2023
Merged
@fedejeanne fedejeanne marked this pull request as ready for review May 31, 2023 13:49
HeikoKlare
HeikoKlare reviewed Jun 16, 2023
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test class is a WorkspaceSessionTest. Thus the intention was that between the original test methods test1 and test2, the platform was shut down and restarted.
So from my point of view either

  • the two test methods should remain to still have a restart of the platform, or
  • the test should not be a WorkspaceSessionTest anymore.

I do not see any reasons for restarting the platform in the original bug report (https://bugs.eclipse.org/bugs/show_bug.cgi?id=297635), so probably this stays a proper regression test for that bug when making a simple ResourceTest out of it.

@fedejeanne
Copy link
Contributor Author

Good point, I changed it to a ResourceTest in c08de7a since I also don't see the point in restarting the platform between tests. I mean I get there might be a memory leak when running a single WS instance for a long time (see Comment#4 and Comment#5) but the tests does not seem to address that issue.

HeikoKlare
HeikoKlare approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. Just two minor comments for possible improvements, which are (from my side) not mandatory to be processed before merging.

...eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/session/TestBug297635.java Outdated Show resolved Hide resolved
...eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/session/TestBug297635.java
@@ -45,26 +53,12 @@ public BundleContext getContext() {
return Platform.getBundle(PI_RESOURCES_TESTS).getBundleContext();
}

public void test1() {
public void testBug() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method becomes much easier to read and comprehend due to this separation of functionality into dedicated methods.
Maybe you could even remove the comments and, if necessary, improve the method names. That could further improve comprehensibility, also w.r.t. the method actually being composed of two chunks that made up the original test methods (install / reinstall).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean:

  1. Rename testBug to testSavedStatesAreDiscardedAfterSnapshot (or do you have another name in mind?)
  2. Remove comment (/* ... */) in reinstallBundle

If not, could you please give me some example(s)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being imprecise: I just meant to remove the comments within the testBug method, i.e., the comments just repeating what the method called in the line after the comment is doing, like

// install a bundle
installBundle();

The method of the test method can remain as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it now, thanks.

I created/renamed some private methods and removed unnecessary comments in testBug in 0347088

@fedejeanne
Fix random failing of TestBug297635 eclipse-platform#460
0347088 
Merge both test methods in the class into 1 and let the class extend
directly from ResourceTest. Extract private methods to improve
readability. Rethrow
exceptions (they let the test fail anyway).
@HeikoKlare HeikoKlare merged commit 0d516c9 into eclipse-platform:master Jun 21, 2023
12 checks passed
@fedejeanne fedejeanne deleted the bugs/460-TestBug297635 branch June 21, 2023 13:31
@HeikoKlare HeikoKlare mentioned this pull request Jun 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vogella vogella vogella left review comments

@HeikoKlare HeikoKlare HeikoKlare approved these changes

@iloveeclipse iloveeclipse iloveeclipse left review comments

@laeubi laeubi laeubi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Random failing of TestBug297635
6 participants
@fedejeanne @vogella @jukzi @HeikoKlare @iloveeclipse @laeubi