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

shutdown dispatcher #4160

Merged
merged 2 commits into from Jan 9, 2021
Merged

shutdown dispatcher #4160

merged 2 commits into from Jan 9, 2021

Conversation

yanns
Copy link
Contributor

@yanns yanns commented Jan 8, 2021

No description provided.

@rossabaker rossabaker closed this Jan 8, 2021
@rossabaker rossabaker deleted the branch http4s:main January 8, 2021 18:48
@rossabaker
Copy link
Member

Incorrectly autoclosed when cats-effect-3 branch was merged.

@rossabaker rossabaker reopened this Jan 8, 2021
@rossabaker rossabaker changed the base branch from cats-effect-3 to main January 8, 2021 18:58
@rossabaker
Copy link
Member

Where does Fixture come from? MUnit?

@RaasAhsan
Copy link
Member

BTW munit-cats-effect has fixture constructors for Resources:

import cats.effect.std.Dispatcher

  val dispatcher = ResourceFixture(Dispatcher[IO])

  dispatcher.test("resources can be lifted to munit fixtures") { dsp =>
    dsp.unsafeRunAndForget(IO(42))
  }

@yanns
Copy link
Contributor Author

yanns commented Jan 8, 2021

Where does Fixture come from? MUnit?

Yes this is coming from MUnit

@yanns
Copy link
Contributor Author

yanns commented Jan 8, 2021

BTW munit-cats-effect has fixture constructors for Resources:

import cats.effect.std.Dispatcher

  val dispatcher = ResourceFixture(Dispatcher[IO])

  dispatcher.test("resources can be lifted to munit fixtures") { dsp =>
    dsp.unsafeRunAndForget(IO(42))
  }

Yes I saw that. Do you think you should prefer that?
It's ok for me. It would mean more tests to adapt.

@rossabaker
Copy link
Member

It seems like the munit-cats-effect version requires a lambda per test? The mixin is syntactically lighter, but too many mixins can get disorienting. I haven't written enough MUnit tests to have a strong opinion which is nicer. I defer to @RaasAhsan or @cquiroz.

Either way, this does look like forward progress.

@RaasAhsan
Copy link
Member

We've been using ResourceFixture wherever we can so far. If you decide to go the ResourceFixture route, see this typelevel/munit-cats-effect#60 . Should be fixed in the next version whenever we upgrade

@yanns
Copy link
Contributor Author

yanns commented Jan 9, 2021

Suggestion:

  1. we can merge this. I'm interesting to see if this change will help to fix flaky tests like Flaky blaze-client test: Use User-Agent header provided in Request  blaze#640
  2. I can work on another version using ResourceFixture in another PR, and we can compare the two approaches

@rossabaker
Copy link
Member

👍 I'm not optimistic it will help that test, but I think it is an improvement, and we can follow up with the ResourceFixture as you suggest.

@rossabaker rossabaker merged commit ef4c9b4 into http4s:main Jan 9, 2021
@yanns yanns deleted the shutdown_dispatcher branch January 9, 2021 16:22
@yanns
Copy link
Contributor Author

yanns commented Jan 9, 2021

The variant with ResourceFixture(Dispatcher[IO]) to compare: #4171

rossabaker added a commit to http4s/http4s-servlet that referenced this pull request Apr 3, 2022
rossabaker pushed a commit to http4s/http4s-servlet that referenced this pull request Apr 3, 2022
following http4s/http4s#4160, try if using `ResourceFixture(Dispatcher[IO])` is better than the current approach.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants