#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

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

…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 12377261111

Details

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

Files with Coverage Reduction	New Missed Lines	%
com/devonfw/tools/ide/process/ProcessResultImpl.java	5	80.0%
com/devonfw/tools/ide/process/ProcessContextImpl.java	24	79.66%

Totals
Change from base Build 12375027220:	0.03%
Covered Lines:	6662
Relevant Lines:	9528

💛 - Coveralls

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

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.

I tried using ConcurrentLinkedQueue , and at first, it worked fine. However, after running my test case several times, I noticed that the order sometimes changed, causing err1 to appear first in the list of output messages.

OMG. Sorry, if I caused a mess here.
Lets have a look at this in January after vacation time.
Maybe we can resolve the problem and make it work deterministic and reliable...
In the worst case, we have to revert the Concurrent-Collection approach and need to go back to your original solution using synchronized (or better using ReentrantLock).

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();

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

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 · 2024-12-17T17:03:08Z

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

@@ -181,7 +182,8 @@ public ProcessResult run(ProcessMode processMode) {
      }
    }
    this.arguments.clear();
-    return new ProcessResultImpl("git", command.toString(), this.exitCode, this.outs, this.errors);
+    setProcessResult(getExitCode(), this.processResult.getOutputMessages());
+    return this.processResult;


setExitCode() and getExitCode() are currently not used in the implementation. The exitcode is set differently in conditions with e.g. this.exitcode = 0

What is here the best solution? Is it the right way to use getters and setters inside a class, also for mock classes or can we just assign the attributes with values directly?

alfeilex · 2024-12-17T17:06:02Z

cli/src/test/java/com/devonfw/tools/ide/process/ProcessContextImplTest.java

+    // assert
+    assertThat(context).log(IdeLogLevel.INFO).hasEntries("out1", "err1", "out2", "err2");
+  }
+


After several runs, you will see that the order is not deterministic, probably because the choice of the first stdout or stderr thread is not fixed.

If synchronized is to inefficent and we stick to ConcurrentLinkedQueue then we have to accept the compromise that the order is not 100% guaranteed. Also, the test-case doesn't work and testing the order like in the implemented test-case isn't possible and has to be removed before merging

hohwille · 2024-12-19T17:13:30Z

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

+          OutputMessage outputMessage = new OutputMessage(errorStream, line);
+          outputMessages.add(outputMessage);
+        }
+
        return br.lines().toList();


why do we try to read the lines again from the buffered reader after it has already been consumed to the end?
IMHO you should change the signature from CompletableFuture<List<String>> to CompletableFuture<Void> and simply return null here.

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 Show resolved Hide resolved

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

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

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

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

alfeilex commented Dec 11, 2024

View reviewed changes

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

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

alfeilex added 2 commits December 16, 2024 22:17

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

8491143

refactor to lazy getter and remove err and out attributes

d7e176e

alfeilex added 4 commits December 17, 2024 08:27

change from synchronized approach to ConcurrentLinkedQueue

ffd6ced

refactor doLog function to output level based stdout and stderr

4c5f98a

add ProcessResult to ProcessContextGitMock and remove redundant

dce5d5a

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

39de3a4

alfeilex commented Dec 17, 2024

View reviewed changes

hohwille reviewed Dec 19, 2024

View reviewed changes

hohwille added this to the release:2025.01.001 milestone Dec 20, 2024

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

coveralls commented Dec 11, 2024 •

edited

Loading

hohwille left a comment

hohwille Dec 12, 2024 •

edited

Loading

alfeilex Dec 17, 2024 •

edited

Loading

hohwille Dec 20, 2024

hohwille Dec 12, 2024

alfeilex Dec 17, 2024

hohwille Dec 12, 2024

hohwille Dec 12, 2024

alfeilex Dec 17, 2024

alfeilex Dec 17, 2024

alfeilex Dec 17, 2024

alfeilex Dec 18, 2024

hohwille Dec 19, 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

coveralls commented Dec 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12377261111

Details

💛 - Coveralls

hohwille left a comment

Choose a reason for hiding this comment

hohwille Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

alfeilex Dec 17, 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

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

alfeilex Dec 17, 2024 •

edited

Loading