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

feat(zio): support Native #432

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

ThijsBroersen
Copy link
Contributor

@ThijsBroersen ThijsBroersen commented Aug 25, 2024

This PR should implement support for Scala Native for the zio module. The zio tests are refactored to zio-tests because running zio's to futures for Scalatest has issues (tests finishing because tests run on different execution context than the zio's). Pure zio-tests work way smoother.

It also fixes test setups for JS, seemed like JS suits were ignored.

closes #431

@adamw
Copy link
Member

adamw commented Sep 2, 2024

Thanks - there are now some test failures in fs2 unfortunately ;)

[info] - should Pass all bytes if limit is not exceeded *** FAILED ***
[info]   java.lang.IllegalStateException: Queue is empty while future is not completed, this means you're probably using a wrong ExecutionContext for your task, please double check your Future.
[info]   at org.scalatest.concurrent.SerialExecutionContext.recRunNow(file:///home/runner/work/sttp-shared/sttp-shared/fs2/target/js-2.13/fs2-test-fastopt/main.js:34463:49)

What's exactly the fix for test setup in JS? Maybe this will lead to some fix of the above ...

@ThijsBroersen
Copy link
Contributor Author

ThijsBroersen commented Sep 3, 2024

I think the JS specs were never executed 🤣 .
FS2 spec should be fixed now.

Btw, had some issues with the JS test setup. Was unable to get it working. I used Playwright temporarily to test it local. Much easier. But this project uses a shared SbtSoftwareMillBrowserTestJS modules for the JS setup. So if you want to switch you might want to do that in that module. -> https://github.com/gmkumar2005/scala-js-env-playwright?tab=readme-ov-file
It was basically replacing the scalajs-env-selenium sbt plugin and adding

  Test / jsEnv := new PWEnv(
    browserName = "chrome",
    headless = true,
    showLogs = true
  )

to the JS test-settings, and off you go.

@adamw
Copy link
Member

adamw commented Sep 3, 2024

@ThijsBroersen There was a very specific reason for using Chrome (some years ago). Maybe it was fetch support? Not sure :) Anyway, that's what we're using in tapir, sttp, though it has its own problems. But maybe this would work now - we'd need to check in all of the projects, though.

import fs2._
import org.scalatest.flatspec.AsyncFlatSpec
import org.scalatest.matchers.should.Matchers
import sttp.capabilities.StreamMaxLengthExceededException

class Fs2StreamsTest extends AsyncFlatSpec with Matchers {

implicit val runtime: unsafe.IORuntime = unsafe.IORuntime(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this is different from the default - could you add a comment how it is different, and why the change is made?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the scalatest suites have an implicit executionContext which is used to schedule the tests. If tests schedule tasks on other executioncontext it is undetectable by scalatest.

@@ -27,7 +27,7 @@ def dependenciesFor(version: String)(deps: (Option[(Long, Long)] => ModuleID)*):
val commonSettings = commonSmlBuildSettings ++ ossPublishSettings ++ Seq(
organization := "com.softwaremill.sttp.shared",
libraryDependencies ++= Seq(
"org.scalatest" %% "scalatest" % scalaTestVersion % Test
"org.scalatest" %%% "scalatest" % scalaTestVersion % Test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that why JS tests were not executed? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@ThijsBroersen
Copy link
Contributor Author

ThijsBroersen commented Sep 3, 2024

I made another commit, dropping zio-test to stay with scalatest. Just overriding the executionContext's of the suites. Let me know if you want to keep the zio-test rewrite or this one (old scalatest implementation).

@adamw
Copy link
Member

adamw commented Sep 4, 2024

I'll keep the scalatest version - thanks! :)

@adamw adamw merged commit e12212a into softwaremill:master Sep 4, 2024
5 checks passed
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.

Feat: support zio for Native
2 participants