Added a extra check in if condition to avoid null pointer exception

[AniketNS]

Testing done

Submitter checklist

Beta Give feedback

1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
2. Ensure that the pull request title represents the desired changelog entry
3. Please describe what you did
4. Link to relevant issues in GitHub or Jira
5. Link to relevant pull requests, esp. upstream and downstream changes
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

This change works fine in my machine when I run command mvn clean -DskipTests verify . The change is already discussed in PR #1646 .

[MarkEWaite]

I still don't understand the logic or why it was added.

Previously, if the step was configured to not use the merge request description, then it would return the value of merge.CommitMessage.

In the new code, if the step was configured to not use the merge request description or if the merge commit message of the step is empty or null, then it would return the value of merge.CommitMessage.

I don't see how that extra condition avoids a null pointer exception. Can you more clearly describe the case where you've seen a null pointer exception?

[MarkEWaite]

Please complete the "Testing done" section of the pull request template. In that section, it says:

Provide a clear description of how this change was tested. At minimum this should include proof that a computer has executed the changed lines. Ideally this should include an automated test or an explanation as to why this change has no tests. Note that automated test coverage is less than complete, so a successful PR build does not necessarily imply that a computer has executed the changed lines. If automated test coverage does not exist for the lines you are changing, you must describe the scenario(s) in which you manually tested the change. For refactoring and code cleanup changes, exercise the code before and after the change and verify the behavior remains the same.

[AniketNS]

I still don't understand the logic or why it was added.

Previously, if the step was configured to not use the merge request description, then it would return the value of merge.CommitMessage.

In the new code, if the step was configured to not use the merge request description or if the merge commit message of the step is empty or null, then it would return the value of merge.CommitMessage.

I don't see how that extra condition avoids a null pointer exception. Can you more clearly describe the case where you've seen a null pointer exception?

Oh, I'm so sorry @MarkEWaite, I forgot to add the condition "if mergeCommitMessage is not null". I just checked if it is null.

Yeah, you are correct,

"Previously, if the step was configured to not use the merge request description, then it would return the value of merge.CommitMessage."

But what if the value of merge.CommitMessage is empty. It would just return an empty message.

That's why I added a code to check if merge.CommitMessage is not null. And if the merge.CommitMessage is null then it won't return it.

That's what I wanted to do. Correct me if I'm wrong.

[AniketNS]

Please complete the "Testing done" section of the pull request template. In that section, it says:

Provide a clear description of how this change was tested. At minimum this should include proof that a computer has executed the changed lines. Ideally this should include an automated test or an explanation as to why this change has no tests. Note that automated test coverage is less than complete, so a successful PR build does not necessarily imply that a computer has executed the changed lines. If automated test coverage does not exist for the lines you are changing, you must describe the scenario(s) in which you manually tested the change. For refactoring and code cleanup changes, exercise the code before and after the change and verify the behavior remains the same.

Sorry, I didn't understand @MarkEWaite, should I provide the screenshot that my computer has executed the changed lines that it has actually, with the command mvn clean -DskipTests verify(without tests)? I haven't tested this new code. I was just going through the code and trying to understand it and spot this correction and thought should make a PR.

Can you please provide me with some more detail on what should I do?

[MarkEWaite]

Can you please provide me with some more detail on what should I do?

A common technique that I've used is to open my IDE, set a breakpoint on the modified line, then run automated tests from the debugger to confirm that the breakpoint is reached. If the breakpoint is reached, then that tells you the code is executed by automated tests. That also offers a hint of the test that might be extended or modified to test for the null pointer exception that you are trying to avoid.

If the breakpoint is not reached by the automated tests, then I'll run Jenkins core and the plugin from a debugger, set a breakpoint on the modified line, and confirm that I can reach that breakpoint interactively.

If the debugger and breakpoint method does not work for you, then you can always locally add logging statements that report when a line is executed.

[AniketNS]

Can you please provide me with some more detail on what should I do?

A common technique that I've used is to open my IDE, set a breakpoint on the modified line, then run automated tests from the debugger to confirm that the breakpoint is reached. If the breakpoint is reached, then that tells you the code is executed by automated tests. That also offers a hint of the test that might be extended or modified to test for the null pointer exception that you are trying to avoid.

If the breakpoint is not reached by the automated tests, then I'll run Jenkins core and the plugin from a debugger, set a breakpoint on the modified line, and confirm that I can reach that breakpoint interactively.

If the debugger and breakpoint method does not work for you, then you can always locally add logging statements that report when a line is executed.

Thanks for the nice explanation. I'll try this and let you know.

[AniketNS]

A common technique that I've used is to open my IDE, set a breakpoint on the modified line, then run automated tests from the debugger to confirm that the breakpoint is reached. If the breakpoint is reached, then that tells you the code is executed by automated tests. That also offers a hint of the test that might be extended or modified to test for the null pointer exception that you are trying to avoid.

Hey @MarkEWaite, I was trying to perform debugging on the AcceptGitLabMergeRequestStep.java file. But, I got to know that there is no test file to debug this file and I cannot perform the debugging without a test file. Do I need to write the test file first or is there anything else I didn't understand?

[krisstern]

Hi @AniketNS, when and where do you encounter a null pointer exception for src/main/java/com/dabsquared/gitlabjenkins/workflow/AcceptGitLabMergeRequestStep.java?

[MarkEWaite]

Closing pull request due to no response. Can be reopened in the future if a response is received