Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace deprecated GenEvent with Erlang's :gen_event #12

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

clifton-mcintosh
Copy link
Collaborator

@clifton-mcintosh clifton-mcintosh commented Mar 29, 2020

The main goal of the work in this change is to replace the deprecated GenEvent with Erlang's :gen_event. In support of that goal, the HTTP handler is set in test using a Mock handler with the Mox library while HTTPPoison is set as the HTTP handler outside of test. With a mock HTTP handler available, tests for Airbrake.Worker.report/1 and Airbrake.Worker.remember/1 are also aded.

Replace deprecated GenEvent with Erlang's :gen_event

The GenEvent behaviour in Elixir is deprecated. When it is in use, Airbrake.LoggerBackend generates the following warning.

warning: GenEvent.__using__/1 is deprecated. Use one of the alternatives described in the documentation for the GenEvent module
  lib/airbrake/logger_backend.ex:3

Using @behaviour :gen_event is one of the suggested ways to replace use GenEvent. That replacement is done here: https://github.com/clifton-mcintosh/airbrake-elixir/blob/use_erlang_gen_event/lib/airbrake/logger_backend.ex#L4.

defmodule Airbrake.LoggerBackend do
  @moduledoc false

  @behaviour :gen_event
...

This is the central change in this pull request.

Tests to increase confidence that replacement is safe

In order to increase confidence that replacing use GenEvent with @behaviour :gen_event in Airbrake.LoggerBackend is safe, tests were added to Airbrake.LoggerBackend prior to changing to verify that when the logger backend is set to Airbrake.LoggerBackend and an error is logged, it is correctly sent to the HTTP handler that is responsible for sending an error to Airbrake.

Once the tests were added (see ae37114), use GenEvent was replaced with @behaviour :gen_event. (See 323f01d).

Tests for Airbrake.Worker

With a test HTTP handler in place, tests were also added for Airbrake.Worker.report/1 and Airbrake.Worker.remember/1. The tests for Airbrake.Worker.report/1 use the test HTTP handler to verify that the expected data are sent when a report is requested.

Calling Airbrake.start() in specific tests

Because Airbrake.Worker.remember/1 alters the state of the Airbrake.Worker module, having one version start up can present difficulties during testing. In particular, sometimes extra data would be present in other tests (for Airbrake.Worker.report/1) when the tests for Airbrake.Worker.remember/1. To remove this problem, testing is set to use the --no-start flag and tests that need Airbrake.start() to run have it explicitly invoked during setup.

Tests for the Airbrake.Worker module start and stop that module before and after each test so that state from one test does not affect the results of another one.

Commit order

GitHub is listing commits out of order. When I run git log locally, this is the order I see:

* 3447672 (HEAD -> use_erlang_gen_event, origin/use_erlang_gen_event) Conditionally start worker
* 1b6b0ba Add test for Airbrake.Worker.remember
* ed71ca9 Explicitly start app for specific tests
* 6bf6d6f add test for Airbrake.Worker.report/1
* 323f01d use "@behaviour :gen_event" instead of "use GenEvent"
* ae37114 Add test for LoggerBackend
* b487b5a format Airbrake.LoggerBackend with "mix format"
* 25c0931 Use environment-specific adapter in Airbrake.Worker
* d74aa48 Set http adapter in environment-specific configs
* 8284c15 define mock for HTTPoison.Base
* 283e163 Add mox and ex_coveralls dependencies for testing
* 8b9ec45 (origin/master, origin/HEAD, master) Update README

That also matches the order of the commits when looking at the branch in GitHub. See https://github.com/clifton-mcintosh/airbrake-elixir/commits/use_erlang_gen_event.

The incorrect order in the pull request may be because I did some rebasing before pushing to GitHub.

@clifton-mcintosh
Copy link
Collaborator Author

I am going to make a small change to this PR because configs from a library are not imported by a project that uses the library. I will close this PR until that change is made. I will re-open it once that change is made.

Configs from a library are not normally imported by a project
that uses that library. This sets HTTPoisong as a default HTTP
handler if/when the config for a handler is not present.
@clifton-mcintosh
Copy link
Collaborator Author

Retrieving the HTTP adapter has been updated to:

  • provide a default of HTTPoison
  • move the config into a "private" element in the config to signal that this is not a configuration option that should be set in a project that uses this library

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.

1 participant