Skip to content

fix log.With() race condition - #64

Merged
aymanbagabas merged 3 commits into
charmbracelet:mainfrom
alexvelea:main
Jul 13, 2023
Merged

aymanbagabas merged 3 commits into
charmbracelet:mainfrom
alexvelea:main

Conversation

@alexvelea

@alexvelea alexvelea commented Jul 12, 2023 •

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

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

codecov Bot commented Jul 12, 2023 •

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

@aymanbagabas, could you try again now, please?

@alexvelea

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@alexvelea

Copy link
Copy Markdown
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

⌂ 换网址