Nebo15 / logger_json

JSON logger formatter with support for Google Cloud, DataDog and other for Elixir.
https://nebo15.github.io/logger_json/
MIT License
237 stars 92 forks source link

`:encoder_opts` formatter option #130

Closed anthonator closed 1 month ago

anthonator commented 1 month ago

Wanted to get a sanity check before moving forward with other formatters to make sure I'm on the right track.

I'm not 100% sure what to do for testing. I basically need to test the behavior of Jason when passing an encoder option which seems clunky and brittle no matter how it gets implemented. At the moment I'm doing a partial regex match on a pretty printed string. Open to other ideas!

bvobart commented 1 month ago

Nice, looks like a pretty sane approach to me (but I'm not @AndrewDryga).

For testing, I think you have two options:

  1. Mock the Jason.encode_to_iodata! function using Mimic during the encoder_opts tests, then simply verify whether the call to Jason includes the encoder options that were set on the formatter earlier. This makes it more of a unit test and you won't need to test Jason's behaviour.
  2. Do it like you're doing now, pick a couple (or a bunch) of Jason encoder options of which you know exactly how they transform the output, then log something and check the output with regex matches. This would make it more of an integration test and therefore also tests whether specifying encoder_opts actually makes a functional difference in log output, but would fail when Jason makes a breaking change to their encoder options, or how they are passed in. That should constitute a major version upgrade on their side, so I doubt its much of a risk (especially looking at Jason's release history, I don't think there's a v2 coming soon).

Maybe both is better for completeness: the integration test for testing that specifying the encoder_opts actually makes a difference in log output, the unit test to ensure that all encoder_opts are being passed through.

anthonator commented 1 month ago

@bvobart thanks for reviewing and providing your thoughts!

Since I'm not the maintainer of this library I'd prefer not to introduce something like mocking. Also, since the integration with Jason is minimal for this feature I think the current approach should be fine for now from a maintainability perspective. Which is my main concern.

I'll keep moving forward with my current approach.

Thanks again for the review! ❤️

anthonator commented 1 month ago

@AndrewDryga this pull request is ready for review.

closes #130

anthonator commented 1 month ago

@AndrewDryga thank you for the review! I updated this pull request with your suggestions.

coveralls commented 1 month ago

Coverage Status

coverage: 100.0%. remained the same when pulling c00876af405606a3f06290e6fa56cac272c2ba63 on anthonator:encoder-opts into 2e72e28185c17e842f4653daa0d556ea92479810 on Nebo15:master.

AndrewDryga commented 1 month ago

Thank you!

anthonator commented 1 month ago

No, thank you! 😄