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

🐛 Ensure that no-token case is actually logged #465

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

webknjaz
Copy link
Contributor

Prior to this patch, there was an unreachable code path that seems to have been intended for marking the case when no token is provided with the "NOTOKEN" string in the debug log output. However the logic behind choosing to show it contained two opposite checks. Said place in the logging utils, didn't have any coverage either.

This change updates the code to make all of its branches reachable. Moreover, it adds test cases for all the branches that remain in the implementation.

@webknjaz
Copy link
Contributor Author

FTR I noticed this unreachable path while working on #464.

@webknjaz
Copy link
Contributor Author

I think I saw some of those red tests being flaky locally for some reason. I'm going to wait until #464 is in and rebase to see if that'll be enough.

Prior to this patch, there was an unreachable code path that seems
to have been intended for marking the case when no token is provided
with the `"NOTOKEN"` string in the debug log output. However the logic
behind choosing to show it contained two opposite checks. Said place
in the logging utils, didn't have any coverage either.

This change updates the code to make all of its branches reachable.
Moreover, it adds test cases for all the branches that remain in the
implementation.
@webknjaz webknjaz force-pushed the bugfixes/token-logging-coverage branch from a06cbdf to 1772372 Compare June 26, 2024 21:38
@webknjaz
Copy link
Contributor Author

🤞

@webknjaz webknjaz marked this pull request as draft June 27, 2024 14:26
@webknjaz
Copy link
Contributor Author

😮‍💨 I'll look into it later..

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

Successfully merging this pull request may close these issues.

1 participant