#1992 improve gitcontextmock by Caylipp · Pull Request #2093 · devonfw/IDEasy

Caylipp · 2026-06-30T07:39:52Z

This PR fixes #1992

Implemented changes:

Improved GitContextMock behavior to better reflect real Git repository state.
Added and updated unit tests for clone, fetch, and update detection.

Testing instructions

Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:

Run GitContextMockTest.
Verify all tests pass.

Checklist for this PR

Make sure everything is checked before merging this PR. For further info please also see
our DoD.

When running mvn clean test locally all tests pass and build is successful
PR title is of the form #«issue-id»: «brief summary» (e.g. #921: fixed setup.bat). If no issue ID exists, title only.
PR top-level comment summarizes what has been done and contains link to addressed issue(s)
PR and issue(s) have suitable labels
Issue is set to In Progress and assigned to you or there is no issue (might happen for very small PRs)
You followed all coding conventions
You have added the issue implemented by your PR in CHANGELOG.adoc unless issue is labeled
with internal
You have formulated clear instructions on how to test your contribution under "Testing instructions"

coveralls · 2026-06-30T07:45:20Z

Coverage Report for CI Build 28948555099

Coverage decreased (-0.02%) to 72.106%

Details

Coverage decreased (-0.02%) from the base build.
Patch coverage: No coverable lines changed in this PR.
1 coverage regression across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
com/devonfw/tools/ide/version/VersionSegment.java	1	91.08%

Coverage Stats


Relevant Lines:	16707
Covered Lines:	12554
Line Coverage:	75.14%
Relevant Branches:	7456
Covered Branches:	4869
Branch Coverage:	65.3%
Branches in Coverage %:	Yes
Coverage Strength:	3.18 hits per line

💛 - Coveralls

maybeec

Thanks for tackling this — the mock now models the full clone → fetch → pull → update-detection lifecycle with a pending-commits queue, and CI is green. Nicely structured and the new GitContextMockTest reads well.

I have two blocking items that come straight from the issue text / project conventions, plus a scope question. Inline comments give one-click suggestions for the convention fix.

Must-fix

Use the project's IniFile support for the .git/config file. #1992 explicitly asks that the config be created "with our IniFile support". Both the write (in clone) and the read (in retrieveGitUrl) are currently hand-rolled. com.devonfw.tools.ide.io.ini.IniFile / IniFileImpl already exist — using them also removes the fragile line parsing (see inline comments).
Exception handling convention. coding-conventions.adoc (§"Catching and handling Exceptions") requires wrapping as throw new IllegalStateException("<meaningful message>", e) — a message and the cause. There are 8 bare throw new RuntimeException(e) sites; I've left commit-able suggestions on each.

Scope question (non-blocking, worth confirming)
The issue closes with: "Our tests using GitContextMock can then be reworked and improved… Also we can properly test features like ide upgrade or the check if update is available." This PR delivers the mock infrastructure and self-tests, but the existing consumer (UpdateCommandletTest) isn't reworked and there are no feature-level tests exercising the new capabilities. Is that intended as a follow-up, or should it land here to fully close #1992?

Minor

GitChange has no equals/hashCode, so GitCommit.hashCode() (which delegates to List<GitChange>.hashCode()) is identity-based — fine for a single run, just not content-stable. See inline note.
java.io.IOException FQN in clone/pull vs. the imported IOException elsewhere is inconsistent (cosmetic).
Heads-up: IdeTestContext defaults to GitContextMock, so flipping these methods from no-ops to active FS operations changes semantics for every consumer (e.g. updateUrls() now creates a .git tree under the urls path). CI is green so nothing breaks today — just noting the widened side-effect surface.

Also needs a rebase — the branch is currently behind main.

maybeec · 2026-07-03T22:37:36Z

+      StringBuilder cfg = new StringBuilder();
+      cfg.append("[remote \"origin\"]\n");
+      cfg.append("\turl = ").append(gitUrl.url()).append('\n');
+      Files.writeString(gitFolder.resolve("config"), cfg.toString());


Per #1992 this config should be produced "with our IniFile support" rather than a hand-built string. com.devonfw.tools.ide.io.ini.IniFileImpl gives you getOrCreateSection for the remote "origin" section plus setting the url property, then write it out. That keeps the format consistent with the real GitContextImpl and makes the round-trip with retrieveGitUrl symmetric.

maybeec · 2026-07-03T22:37:36Z

+      try {
+        for (String line : Files.readAllLines(cfg)) {
+          String trimmed = line.trim();
+          if (trimmed.startsWith("url =")) {


Two issues here, both solved by reading via IniFile/IniFileImpl (see the clone comment):

This returns the first line starting with url = regardless of section, so it isn't scoped to [remote "origin"] — with multiple remotes it would pick the wrong one.

It re-implements ini parsing by hand, which Improve GitContextMock #1992 explicitly asked to avoid.

Read the origin section's url property instead, falling back to MOCKED_URL_VALUE when no config exists.

maybeec · 2026-07-03T22:37:36Z

+      cfg.append("\turl = ").append(gitUrl.url()).append('\n');
+      Files.writeString(gitFolder.resolve("config"), cfg.toString());
+    } catch (java.io.IOException e) {
+      throw new RuntimeException(e);


Follow the project convention for caught exceptions. The same fix applies to every other throw new RuntimeException(e) in this file (lines 135, 158, 190, 257, 280, 313, 355) — each has its own suggestion below.

Suggested change

throw new RuntimeException(e);

throw new IllegalStateException("Failed to clone Git repository " + gitUrl + " to " + repository, e);

📖 Convention: coding-conventions.adoc → "Catching and handling Exceptions" — always wrap a caught exception as IllegalStateException("<meaningful message>", e) carrying the original cause e. A bare throw new RuntimeException(e) violates this: it uses the wrong type and, without a message, discards diagnostic context.

maybeec · 2026-07-03T22:37:36Z

+      Files.writeString(branchRefPath, lastId);
+      Files.writeString(gitFolder.resolve(FILE_FETCH_HEAD), lastId);
+    } catch (java.io.IOException e) {
+      throw new RuntimeException(e);


Suggested change

throw new RuntimeException(e);

throw new IllegalStateException("Failed to pull repository " + repository, e);

📖 Convention: coding-conventions.adoc → "Catching and handling Exceptions" — always wrap a caught exception as IllegalStateException("<meaningful message>", e) carrying the original cause e. A bare throw new RuntimeException(e) violates this: it uses the wrong type and, without a message, discards diagnostic context.

maybeec · 2026-07-03T22:37:36Z

+      Files.createDirectories(gitFolder);
+      Files.writeString(gitFolder.resolve(FILE_FETCH_HEAD), String.valueOf(last.hashCode()));
+    } catch (IOException e) {
+      throw new RuntimeException(e);


Suggested change

throw new RuntimeException(e);

throw new IllegalStateException("Failed to fetch repository " + repository, e);

📖 Convention: coding-conventions.adoc → "Catching and handling Exceptions" — always wrap a caught exception as IllegalStateException("<meaningful message>", e) carrying the original cause e. A bare throw new RuntimeException(e) violates this: it uses the wrong type and, without a message, discards diagnostic context.

maybeec · 2026-07-03T22:37:36Z

+      }
+      return !f.equals(h);
+    } catch (IOException e) {
+      throw new RuntimeException(e);


Suggested change

throw new RuntimeException(e);

throw new IllegalStateException("Failed to check for repository update in " + repository, e);

📖 Convention: coding-conventions.adoc → "Catching and handling Exceptions" — always wrap a caught exception as IllegalStateException("<meaningful message>", e) carrying the original cause e. A bare throw new RuntimeException(e) violates this: it uses the wrong type and, without a message, discards diagnostic context.

maybeec · 2026-07-03T22:37:36Z

+      String t = Files.readString(trackedCommitIdPath).trim();
+      return !f.equals(t);
+    } catch (IOException e) {
+      throw new RuntimeException(e);


Suggested change

throw new RuntimeException(e);

throw new IllegalStateException("Failed to check for repository update in " + repository, e);

📖 Convention: coding-conventions.adoc → "Catching and handling Exceptions" — always wrap a caught exception as IllegalStateException("<meaningful message>", e) carrying the original cause e. A bare throw new RuntimeException(e) violates this: it uses the wrong type and, without a message, discards diagnostic context.

maybeec · 2026-07-03T22:37:36Z

+      // fallback: return content
+      return content;
+    } catch (IOException e) {
+      throw new RuntimeException(e);


Suggested change

throw new RuntimeException(e);

throw new IllegalStateException("Failed to determine current branch for repository " + repository, e);

📖 Convention: coding-conventions.adoc → "Catching and handling Exceptions" — always wrap a caught exception as IllegalStateException("<meaningful message>", e) carrying the original cause e. A bare throw new RuntimeException(e) violates this: it uses the wrong type and, without a message, discards diagnostic context.

maybeec · 2026-07-03T22:37:36Z

+            Files.copy(s, dest, StandardCopyOption.REPLACE_EXISTING);
+          }
+        } catch (IOException e) {
+          throw new RuntimeException(e);


Suggested change

throw new RuntimeException(e);

throw new IllegalStateException("Failed to copy directory " + source + " to " + target, e);

📖 Convention: coding-conventions.adoc → "Catching and handling Exceptions" — always wrap a caught exception as IllegalStateException("<meaningful message>", e) carrying the original cause e. A bare throw new RuntimeException(e) violates this: it uses the wrong type and, without a message, discards diagnostic context.

maybeec · 2026-07-03T22:37:36Z

+  /**
+   * Represents a file or directory change to apply when pulling a commit.
+   */
+  public static class GitChange {


Minor: GitChange overrides neither equals nor hashCode, so GitCommit.hashCode() (delegating to List<GitChange>.hashCode()) ends up identity-based. That's fine for distinguishing "before vs. after" state within one test run, but two commits with identical content get different ids and ids aren't reproducible across runs. Consider making GitChange/GitCommit records (or adding equals/hashCode) if you want content-stable ids.

maybeec

Requesting changes — there are blocking items that should be resolved before merge (details are in the inline comments on the previous review):

Blocking

Use the project's IniFile support for .git/config (write in clone, read in retrieveGitUrl). #1992 explicitly requires this, and it removes the fragile hand-rolled parsing that currently isn't scoped to the [remote "origin"] section.
Exception handling convention — replace the 8 throw new RuntimeException(e) sites with IllegalStateException("<message>", e) (message + cause), per coding-conventions.adoc. One-click suggestions are provided on each line.

Please also clarify (scope of #1992)

The issue asks that the existing tests using GitContextMock be reworked and that features like ide upgrade / update-detection be properly tested with the new mock. That part isn't in this PR — is it a planned follow-up, or should it land here to fully close #1992?

Once the two blocking items are addressed (and the branch rebased onto main), I'm happy to re-review. Thanks again — the core lifecycle modelling is solid.

Caylipp · 2026-07-08T14:06:56Z

Requesting changes — there are blocking items that should be resolved before merge (details are in the inline comments on the previous review):

Blocking

Use the project's IniFile support for .git/config (write in clone, read in retrieveGitUrl). Improve GitContextMock #1992 explicitly requires this, and it removes the fragile hand-rolled parsing that currently isn't scoped to the [remote "origin"] section.

Exception handling convention — replace the 8 throw new RuntimeException(e) sites with IllegalStateException("<message>", e) (message + cause), per coding-conventions.adoc. One-click suggestions are provided on each line.

Please also clarify (scope of #1992)

The issue asks that the existing tests using GitContextMock be reworked and that features like ide upgrade / update-detection be properly tested with the new mock. That part isn't in this PR — is it a planned follow-up, or should it land here to fully close Improve GitContextMock #1992?

Once the two blocking items are addressed (and the branch rebased onto main), I'm happy to re-review. Thanks again — the core lifecycle modelling is solid.

Thanks for the review.

I addressed the two blocking items:

.git/config is now written using IniFile/IniFileImpl, and retrieveGitUrl reads the url from the remote "origin" section.
The bare RuntimeException usages were replaced with IllegalStateException including meaningful messages and the original cause.

I also added equals/hashCode for GitChange and GitCommit to address the minor hash stability note.

Regarding the scope question: I intended this PR to focus on the GitContextMock infrastructure and its self-tests. Reworking consumer tests such as UpdateCommandletTest would be a follow-up, unless you prefer to include it here.

github-project-automation Bot added this to IDEasy board Jun 30, 2026

github-project-automation Bot moved this to 🆕 New in IDEasy board Jun 30, 2026

Caylipp added enhancement New feature or request test related to testing and QA git git version management tool integration internal Nothing to be added to CHANGELOG, only internal story labels Jun 30, 2026

Caylipp self-assigned this Jun 30, 2026

Caylipp moved this from 🆕 New to Team Review in IDEasy board Jun 30, 2026

Caylipp assigned KarimALotfy Jul 1, 2026

maybeec reviewed Jul 3, 2026

View reviewed changes

maybeec requested changes Jul 3, 2026

View reviewed changes

Caylipp added 4 commits July 8, 2026 10:54

devonfw#1992: Initial draft

1496c91

devonfw#1992: initial improved GitContextMock

a7a5f1e

devonfw#1992: add Junit tests for GitContextMock

de967ae

devonfw#1992: add CHANGELOG.adoc entry

0d37f75

Caylipp force-pushed the feature/1992-improve-gitcontextmock branch from 7fe0b65 to 0d37f75 Compare July 8, 2026 09:00

devonfw#1992: addressing review

036f9bd

	throw new RuntimeException(e);
	throw new IllegalStateException("Failed to clone Git repository " + gitUrl + " to " + repository, e);

Uh oh!

Conversation

Caylipp commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR fixes #1992

Implemented changes:

Testing instructions

Checklist for this PR

Uh oh!

coveralls commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 28948555099

Coverage decreased (-0.02%) to 72.106%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

maybeec left a comment

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maybeec Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

maybeec left a comment

Choose a reason for hiding this comment

Uh oh!

Caylipp commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Caylipp commented Jun 30, 2026 •

edited

Loading

coveralls commented Jun 30, 2026 •

edited

Loading

maybeec Jul 3, 2026 •

edited

Loading

maybeec Jul 3, 2026 •

edited

Loading

maybeec Jul 3, 2026 •

edited

Loading

maybeec Jul 3, 2026 •

edited

Loading

maybeec Jul 3, 2026 •

edited

Loading

maybeec Jul 3, 2026 •

edited

Loading

maybeec Jul 3, 2026 •

edited

Loading