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

fix log.With() race condition #64

Merged
merged 3 commits into from
Jul 13, 2023
Merged

Conversation

alexvelea
Copy link
Contributor

@alexvelea alexvelea commented Jul 12, 2023

  • added small unittest to highlight the problem
  • problem only visible when running with -race option

Verified

This commit was signed with the committer’s verified signature. The key has expired.
BendingBender Dimitri B.
- added small unittest to highlight the problem
- only visible when running with -race option
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #64 (e11ecdd) into main (31d2a53) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
+ Coverage   76.77%   76.85%   +0.07%     
==========================================
  Files          10       10              
  Lines         633      635       +2     
==========================================
+ Hits          486      488       +2     
  Misses        132      132              
  Partials       15       15              
Impacted Files Coverage Δ
logger.go 82.75% <100.00%> (+0.17%) ⬆️

@aymanbagabas
Copy link
Member

Hi @alexvelea, thanks for the PR. I'm not able to reproduce the issue on main using go test -race ./..., I'm using macOS to test this.

@alexvelea
Copy link
Contributor Author

Hi @alexvelea, thanks for the PR. I'm not able to reproduce the issue on main using go test -race ./..., I'm using macOS to test this.

Thanks for the fast reply @aymanbagabas
Did you undo the fix before? (from the logger.go file)
I'm using the same command on my end. macOS M1

@aymanbagabas
Copy link
Member

Thanks for the fast reply @aymanbagabas Did you undo the fix before? (from the logger.go file) I'm using the same command on my end. macOS M1

I'm testing it against the latest commit from main:

~/Source/charmbracelet/log main*
› git rev-parse HEAD
31d2a537b4d27b2d45169051aeb48b35c2712398

~/Source/charmbracelet/log main*
› go test -race ./...
ok      github.com/charmbracelet/log    0.241s

@alexvelea
Copy link
Contributor Author

@aymanbagabas, could you try again now, please?

@alexvelea
Copy link
Contributor Author

Also, make sure to run the tests on this branch.
The main branch passes, but I've added a new test here (as well a fix, which is now rollbacked)

@aymanbagabas aymanbagabas requested a review from caarlos0 July 12, 2023 18:39
@aymanbagabas
Copy link
Member

@alexvelea Ahh I see the issue now, could you re-push your changes again?

@alexvelea
Copy link
Contributor Author

@aymanbagabas done, tests are fixed now

@aymanbagabas aymanbagabas merged commit e0ec0b1 into charmbracelet:main Jul 13, 2023
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.

2 participants