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.

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

NPE during logging #307

Merged
merged 2 commits into from
Apr 12, 2022
Merged

NPE during logging #307

merged 2 commits into from
Apr 12, 2022

Conversation

remcowesterhoud
Copy link
Contributor

@remcowesterhoud remcowesterhoud commented Apr 11, 2022

Description

Variable with a null value produce NPE when logging the record stream. This PR adds a null check and a bunch of tests for our logger to prevent this in the future.

Related issues

closes #305

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually

Documentation:

  • Javadoc has been written
  • The documentation is updated

Variable with a value of null would produce a NPE during the logging of the record stream. This nullcheck fixes this issue.
@github-actions
Copy link

github-actions bot commented Apr 11, 2022

Unit Test Results

136 files  136 suites   6m 31s ⏱️
246 tests 246 ✔️ 0 💤 0
940 runs  940 ✔️ 0 💤 0

Results for commit ba7227d.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@pihme pihme left a comment

Choose a reason for hiding this comment

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

Change looks good. I think the tests should be looked at again.

@remcowesterhoud remcowesterhoud force-pushed the 305_NPE_during_logging branch from 4ffde56 to 53e0adb Compare April 12, 2022 08:09
This class was mostly not tested. We tested if we had a logger for all records, but we did not test the fucntionality of the logger itself. It doesn't contain much logic but by adding some tests around it we can hopefully prevent NPE's in the future.
@remcowesterhoud remcowesterhoud force-pushed the 305_NPE_during_logging branch from 53e0adb to ba7227d Compare April 12, 2022 08:11
@remcowesterhoud remcowesterhoud requested a review from pihme April 12, 2022 08:12
@remcowesterhoud remcowesterhoud merged commit 6ded9a3 into main Apr 12, 2022
@remcowesterhoud remcowesterhoud deleted the 305_NPE_during_logging branch April 12, 2022 08:25
@github-actions
Copy link

Successfully created backport PR #310 for stable/8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE in RecordStreamLogger.logVariables(
2 participants