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

Add tests for the request logging #3923

Closed
yashrsharma44 opened this issue Mar 12, 2021 · 5 comments
Closed

Add tests for the request logging #3923

yashrsharma44 opened this issue Mar 12, 2021 · 5 comments

Comments

@yashrsharma44
Copy link
Contributor

PRs for request logging #3361 #2849 contains some string manipulation, which might create unnecessary panic as observed in #3921

Would be nice to write tests for making sure panics do not happen.

@ashish493
Copy link

Any type of checklist for solving this issue?
Like do we have to use some tesing framework like ginkgo or like create some basic functional tests like this

func TestPanic(t *testing.T) {
    defer func() {
        if r := recover(); r == nil {
            t.Errorf("The code did not panic")
        }
    }()

    // The following is the code under test
    OtherFunctionThatPanics()
}

I'm bit confused :(

@yashrsharma44
Copy link
Contributor Author

Like do we have to use some tesing framework like ginkgo

We could start with making simple Go-tests, no need to look for any testing framework.

We can also write up unit tests for different functions out in the logging package

  • For starters, you can take a look at the grpc.go, and add unit tests for the functions that are working, would be nice to have a defined behaviour for them

@stale
Copy link

stale bot commented Jun 5, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jun 5, 2021
@onprem onprem removed the stale label Jun 5, 2021
@stale
Copy link

stale bot commented Aug 4, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Aug 4, 2021
@stale
Copy link

stale bot commented Aug 21, 2021

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants