go-approval-tests icon indicating copy to clipboard operation
go-approval-tests copied to clipboard

Published 20 hours ago • verified publisher icon approvals

R - move findFileName from FuncForPC to CallerFrames

Open karngyan opened this issue 2 years ago • 5 comments

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 avatar Jun 08 '22 17:06 karngyan

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?

karngyan avatar Aug 18 '22 17:08 karngyan

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

hjwk avatar Aug 22 '22 16:08 hjwk

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

karngyan avatar Aug 23 '22 13:08 karngyan

@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?

aleclerc-cio avatar Mar 26 '24 14:03 aleclerc-cio

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

hjwk avatar Apr 11 '24 08:04 hjwk