#734: Improve Process Result Model #773

alfeilex · 2024-11-19T07:43:44Z

Fixes: #734

This PR improves the process result capturing of stdout and stderr output by preserving the order of the output.

…om/alfeilex/IDEasy into devonfw#734-improve-process-result

cli/src/main/java/com/devonfw/tools/ide/process/LogEvent.java

+ */
+public record LogEvent(boolean error, String message) {
+
+}


cli/src/main/java/com/devonfw/tools/ide/process/ProcessContextImpl.java

alfeilex · 2024-11-19T19:27:43Z

cli/src/main/java/com/devonfw/tools/ide/process/ProcessContextImpl.java

+          synchronized (logs) {
+            LogEvent logEvent = new LogEvent(errorStream, line);
+            logs.add(logEvent);
+          }


I am not sure, if I am using LogEvent correctly. In this implementation, a single LogEvent instance represents one line from either stout or sterr. I don't see another solution to capture the entire log while keeping the order of the log streams.

Now its OutputMessage instead of LogEvent. We can still discuss the name

…rocess-result # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/process/ProcessContextImpl.java # cli/src/main/java/com/devonfw/tools/ide/process/ProcessResultImpl.java

…age and refactor ProcessContextGitMock

coveralls · 2024-12-11T09:43:22Z

Pull Request Test Coverage Report for Build 12293812522

Details

0 of 0 changed or added relevant lines in 0 files are covered.
22 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.03%) to 67.146%

Files with Coverage Reduction	New Missed Lines	%
com/devonfw/tools/ide/process/ProcessResultImpl.java	5	78.95%
com/devonfw/tools/ide/process/ProcessContextImpl.java	17	80.26%

Totals
Change from base Build 12293756306:	0.03%
Covered Lines:	6632
Relevant Lines:	9532

💛 - Coveralls

alfeilex · 2024-12-11T14:06:20Z

cli/src/main/java/com/devonfw/tools/ide/process/ProcessResultImpl.java

-    this.err = Objects.requireNonNullElse(err, Collections.emptyList());
+    this.outputMessages = Objects.requireNonNullElse(outputMessages, Collections.emptyList());
+    this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).collect(Collectors.toList());
+    this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());
  }


Should we fill the out and err lists in the constructor or elsewhere?

Good point:
We could compute out and err lazily in the getters so if these getters are never called and we have a larger output, this may be pointless waste otherwise...

alfeilex · 2024-12-11T14:08:37Z

cli/src/main/java/com/devonfw/tools/ide/process/ProcessResultImpl.java

-    if (!this.err.isEmpty()) {
-      doLog(errorLevel, this.err, context);
+    if (!this.outputMessages.isEmpty()) {
+      doLog(outLevel, getOutputMessages().stream().map(OutputMessage::message).collect(Collectors.toList()), context);
    }


Are two separate log levels still necessary with a single lists of captures?

There are special cases at least Eclipse for where we want to make err visible to the user but have out on debug that is hidden by default but can be enabled.
I would therefore recommend to:

first check if outLevel == errorLevel and then do as you suggest/implemented

otherwise simply use the old approach but then also first check if the log-level is actually enabled. So if it is disabled, we do nothing (not even calling getOut() or getErr() what would compute the extra List).

hohwille

@alfeilex thanks for your PR. Already on its way and looking good. Thanks 👍
I left some review comments for improvement. Please have a look.

hohwille · 2024-12-12T10:20:59Z

cli/src/main/java/com/devonfw/tools/ide/process/ProcessContextImpl.java

+
+        String line;
+        while ((line = br.readLine()) != null) {
+          synchronized (outputMessages) {


~~This may create a DeadLock.~~ Java default synchronization is very inefficient and we will have two threads continuously blocking each other.
Consider using a concurrent collection.
I would suggest to use ConcurrentLinkedQueue for collecting the OutputMessages and when completed, convert to a regular List.

hohwille · 2024-12-12T10:29:42Z

cli/src/main/java/com/devonfw/tools/ide/process/ProcessResultImpl.java

+    this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).collect(Collectors.toList());
+    this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());


Small simplification:

Suggested change

this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).collect(Collectors.toList());

this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());

this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).toList();

this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).toList();

hohwille · 2024-12-12T10:30:41Z

cli/src/main/java/com/devonfw/tools/ide/process/ProcessResultImpl.java

-    this.err = Objects.requireNonNullElse(err, Collections.emptyList());
+    this.outputMessages = Objects.requireNonNullElse(outputMessages, Collections.emptyList());
+    this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).collect(Collectors.toList());
+    this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());
  }


Good point:
We could compute out and err lazily in the getters so if these getters are never called and we have a larger output, this may be pointless waste otherwise...

hohwille · 2024-12-12T10:34:25Z

cli/src/main/java/com/devonfw/tools/ide/process/ProcessResultImpl.java

-    if (!this.err.isEmpty()) {
-      doLog(errorLevel, this.err, context);
+    if (!this.outputMessages.isEmpty()) {
+      doLog(outLevel, getOutputMessages().stream().map(OutputMessage::message).collect(Collectors.toList()), context);
    }


There are special cases at least Eclipse for where we want to make err visible to the user but have out on debug that is hidden by default but can be enabled.
I would therefore recommend to:

first check if outLevel == errorLevel and then do as you suggest/implemented

otherwise simply use the old approach but then also first check if the log-level is actually enabled. So if it is disabled, we do nothing (not even calling getOut() or getErr() what would compute the extra List).

hohwille · 2024-12-12T10:42:15Z

cli/src/test/java/com/devonfw/tools/ide/context/ProcessContextGitMock.java

   */
  public List<String> getErrors() {

-    return errors;
+    return this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());


This will not work. Current code is calling getErrors() or getOuts() in order to add mocked messages to such List.
Streams will create immutable collections that cannot be modified after creation.
In case we leave it like this, also use toList() simplification in all places.

Could you discuss with @jan-vcapgemini who IMHO created this class about the design?
Wouldn't it be smarter to create a regular ProcessResult here and make it accessible so instead of tests calling
processContextGitMock.getErrors().add("fake error message") we could call processContextGitMock.getProcessResult().getOutputMessages().add(new OutputMessage("fake error message", true))
to make it work.
There is a lot of pointless redundancy in this class related to ProcessResultImpl that can be avoided this way.

BTW: I just noticed that you already found this problem and had to update an existing test-case.

If you want a convenience method for adding faked out or err messages, you could still provide them here but delegating to the ProcessResult. Also this way we could even allow a setter for the mocked ProcessResult that also gives us more flexibility (resetting the result for a next faked git call, etc.).

alfeilex added 2 commits November 19, 2024 08:42

Add single log list

e25575f

Merge branch 'devonfw#734-improve-process-result' of https://github.c…

8e66739

…om/alfeilex/IDEasy into devonfw#734-improve-process-result

hohwille assigned alfeilex Nov 19, 2024

alfeilex added 4 commits November 19, 2024 11:16

Merge branch 'devonfw:main' into devonfw#734-improve-process-result

5c3cccb

Add LogEvent Record

4d5a556

Add LogEvent List to capture Logs

daf4b45

Merge branch 'devonfw#734-improve-process-result' of https://github.c…

05879d1

…om/alfeilex/IDEasy into devonfw#734-improve-process-result

alfeilex commented Nov 19, 2024

View reviewed changes

cli/src/main/java/com/devonfw/tools/ide/process/LogEvent.java Outdated

*/

public record LogEvent(boolean error, String message) {

}

This comment was marked as outdated.

Sign in to view

alfeilex commented Nov 19, 2024

View reviewed changes

cli/src/main/java/com/devonfw/tools/ide/process/ProcessContextImpl.java Outdated Show resolved Hide resolved

alfeilex commented Nov 19, 2024

View reviewed changes

alfeilex added 9 commits November 20, 2024 13:50

Merge branch 'devonfw:main' into devonfw#734-improve-process-result

292aa5b

Merge remote-tracking branch 'origin/main' into devonfw#734-improve-p…

41a42ce

…rocess-result # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/process/ProcessContextImpl.java # cli/src/main/java/com/devonfw/tools/ide/process/ProcessResultImpl.java

add general List of output messages and rename logevent to outputmess…

3da75c7

…age and refactor ProcessContextGitMock

Merge branch 'devonfw:main' into devonfw#734-improve-process-result

540a9b0

refactor

c2b7021

refactor processResultImpl in tooldummy

4530860

merge

05da168

change List of Strings for getOutputMessages

88c8ef1

update Test with output message list

4700583

add test

f0c5b9e

alfeilex commented Dec 11, 2024

View reviewed changes

remove forgotten outs in ProcessContextGitMock

4daec09

alfeilex marked this pull request as ready for review December 11, 2024 14:19

alfeilex and others added 2 commits December 11, 2024 16:59

add CHANGELOG.adoc

4bbb603

Merge branch 'main' into devonfw#734-improve-process-result

af6add1

hohwille reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#734: Improve Process Result Model #773

#734: Improve Process Result Model #773

alfeilex commented Nov 19, 2024 •

edited

Loading

This comment was marked as outdated.

alfeilex Nov 19, 2024

alfeilex Dec 11, 2024

coveralls commented Dec 11, 2024 •

edited

Loading

alfeilex Dec 11, 2024

hohwille Dec 12, 2024

alfeilex Dec 11, 2024

hohwille Dec 12, 2024

hohwille left a comment

hohwille Dec 12, 2024 •

edited

Loading

hohwille Dec 12, 2024

hohwille Dec 12, 2024

hohwille Dec 12, 2024

hohwille Dec 12, 2024

hohwille Dec 12, 2024

		this.out = this.outputMessages.stream().filter(outputMessage -> !outputMessage.error()).map(OutputMessage::message).collect(Collectors.toList());
		this.err = this.outputMessages.stream().filter(OutputMessage::error).map(OutputMessage::message).collect(Collectors.toList());

#734: Improve Process Result Model #773

Are you sure you want to change the base?

#734: Improve Process Result Model #773

Conversation

alfeilex commented Nov 19, 2024 • edited Loading

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12293812522

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hohwille left a comment

Choose a reason for hiding this comment

hohwille Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfeilex commented Nov 19, 2024 •

edited

Loading

coveralls commented Dec 11, 2024 •

edited

Loading

hohwille Dec 12, 2024 •

edited

Loading