search

approvals / go-approval-tests

Apache License 2.0
86 stars 22 forks source link

Fix getApprovalName for Windows #22

Closed hjwk closed 3 years ago

hjwk commented 3 years ago

Fixes #21

emilybache commented 3 years ago

That code change fixes the unit tests but I think it will break the desired production behaviour. I don't have a windows machine to test it out on. The thing to do would be to write some parameterized tests and see whether they still work and create a new file for each parameterized test.

hjwk commented 3 years ago

I had another look and I think just using '/' instead of os.PathSeparator makes more sense because t,Name() does not have anything to do with the underlying OS. It just uses the '/' character, independently of the OS. Will add some tests nonetheless

emilybache commented 3 years ago

Ah, yes. If the behaviour of t.Name() is not platform dependent then as you say using "/" would work better.