search

approvals / go-approval-tests

Apache License 2.0
86 stars 22 forks source link

R - move findFileName from FuncForPC to CallerFrames #41

Open karngyan opened 2 years ago

karngyan commented 2 years ago

Closes 40

Description

Go runtime docs discourage traversing PC or FuncForPC on any returned PCs as they can't account for inlining or return program counter adjustment. This leads to wrong file names for approved and received files.

This PR refactors the implementation of findFileName. I came across this issue while working on one of our approval tests at my company and have been struggling to reproduce it using simple examples. But it did solve the issue we faced.

The solution

Implement the method with CallerFrames instead.

karngyan commented 2 years ago

Looks good but could you rebase ? I found that it breaks a test that was introduced recently because findFileName becomes case sensitive

Which test case are you talking about, and also what recent change?

hjwk commented 2 years ago

Which test case are you talking about, and also what recent change?

TestVerifyJSONBytesWithRegexScrubber fails scrubber_test.go:63: Failed Approval: received does not match approved.

Tested on linux (Pop_os) and go 1.18.3

karngyan commented 2 years ago

scrubber_test.go:63: Failed Approval: received does not match approved.

Hey, I think it's rebased already. I don't see any test failures. Can you please confirm you're running the right branch?

Screenshot 2022-08-23 at 7 22 17 PM

Even ended up running the workflow which passed as well: https://github.com/karngyan/go-approval-tests/actions/runs/2911761732

aleclerc-cio commented 5 months ago

@hjwk I know there hasn't been a commit to this project in a little while. Is there any chance of getting this PR merged?

hjwk commented 4 months ago

sorry, I don't have write access @isidore ?