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.

Sign up for GitHub

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

Jump to bottom

Fix: stack trace of exceptions are not being logged #1974 #3157

Merged
merged 4 commits into from
Oct 11, 2020
Merged

Fix: stack trace of exceptions are not being logged #1974 #3157

merged 4 commits into from
Oct 11, 2020

Conversation

nitram509
Copy link
Contributor

@nitram509 nitram509 commented Oct 9, 2020 •
edited
Loading

Proposed changes in this pull request

This is a fix for #1974 Stack trace of exceptions are not being logged.

  • I did some regex search and replaced all ca. 50 occurrences I found, where " + e" was used instead of ", e" when logging.
    • Used regex expressions: '\Wlog.(.*\s+\se' and simply '\s+\se'
  • I also added some ", e" parameters to Exceptions and Log statements, where I thought it might be useful.
  • Additionally I fixed a test, where some error logging will not make the test fail, but a RuntimeException is preferred to fail fast.
  • Also, there are some trailing whitespaces removed (this is automatic by my IDE), I do apologize for this noise. If you don't like it, I can move them back in.

When should this PR be merged

I don't see any pre-conditions.
Could be merged right away.

Follow up actions

  • a review would be nice
  • feedback would be nice
  • setting a label "hacktoberfest-accepted" would be awesome.

Checklist (for reviewing)

General

  • Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
  • Is the PR labeled correctly?

Functionality

  • Are all requirements met? Compare implemented functionality with the requirements specification.
  • Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

  • Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
  • Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
  • Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

  • Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
  • If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
  • If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

  • Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
  • Are all UI and API inputs run through forms or serializers?
  • Are all external inputs validated and sanitized appropriately?
  • Does all branching logic have a default case?
  • Does this solution handle outliers and edge cases gracefully?
  • Are all external communications secured and restricted to SSL?

Documentation

  • Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
  • Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
  • Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

@CLAassistant
Copy link

CLAassistant commented Oct 9, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@nitram509
fixed test-setup for sampleFunction3
7e058dd 
replace log.error() with Assert.fail() in order to detect test failures
Logging in a test might be overseen
@nitram509
Copy link
Contributor Author

Dear reviewer,
I realized my first push did reveal an overseen Exeception thrown in the test class FunctionLibraryManagementServiceTest.
After reading the code, I concluded, there must be a mistake in the data provider setup, for "sampleFunction3".
Hence I fixed it.
Also, I did replace all log statements with Asster.fail() code, because this would make a test fail instead of silently logging.

I hope that's in your interest.

Any feedback welcome.

pulasthi7
pulasthi7 reviewed Oct 10, 2020
View reviewed changes
Copy link
Contributor

@pulasthi7 pulasthi7 left a comment

Choose a reason for hiding this comment

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

Hi @nitram509

Thanks for your contribution. Much Appreciated.

I'm adding one minor comment to be fixed. Please get that addressed and the PR would be ready to be merged.

...ava/org/wso2/carbon/identity/functions/library/mgt/FunctionLibraryManagementServiceTest.java Outdated
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

As a practice, we avoid * imports. So, please revert this back to individual imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pulasthi7 Thank you for your review.
I did change the import as per convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @nitram509

@pulasthi7 pulasthi7 merged commit 2600c52 into wso2:master Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pulasthi7 pulasthi7 pulasthi7 approved these changes

@aaujayasena aaujayasena Awaiting requested review from aaujayasena

@Abilashini Abilashini Awaiting requested review from Abilashini

@ayshsandu ayshsandu Awaiting requested review from ayshsandu

@Buddhimah Buddhimah Awaiting requested review from Buddhimah

@denuwanthi denuwanthi Awaiting requested review from denuwanthi

@dewniMW dewniMW Awaiting requested review from dewniMW

@DMHP DMHP Awaiting requested review from DMHP

@emswbandara emswbandara Awaiting requested review from emswbandara

@GDRDABARERA GDRDABARERA Awaiting requested review from GDRDABARERA

@IsuraD IsuraD Awaiting requested review from IsuraD

@janakamarasena janakamarasena Awaiting requested review from janakamarasena

@Kanapriya Kanapriya Awaiting requested review from Kanapriya

@mefarazath mefarazath Awaiting requested review from mefarazath

@nilasini nilasini Awaiting requested review from nilasini

@NilukaSripalim NilukaSripalim Awaiting requested review from NilukaSripalim

@omindu omindu Awaiting requested review from omindu

@pamodaaw pamodaaw Awaiting requested review from pamodaaw

@piraveena piraveena Awaiting requested review from piraveena

@sachiniWettasinghe sachiniWettasinghe Awaiting requested review from sachiniWettasinghe

@senthalan senthalan Awaiting requested review from senthalan

@ShanikaWickramasinghe ShanikaWickramasinghe Awaiting requested review from ShanikaWickramasinghe

@somindatommy somindatommy Awaiting requested review from somindatommy

@thanujalk thanujalk Awaiting requested review from thanujalk

@vihanga-liyanage vihanga-liyanage Awaiting requested review from vihanga-liyanage

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nitram509 @CLAassistant @pulasthi7