Adding tests in CI

[TonioGela]

After some brainstorming with @armanbilge, I added some tests to the CI to compile some scripts using scala-cli.

The whole logic is simple:

• toolkitTesting / Test / test depends on toolkit.jvm / publishLocal so that before running the tests the artifact gets published locally
• BuildInfo was added to have both scalaversion and version available in the test scope
• scala-cli was added as a dependency and it's called like ScalaCli.main(Array("compile", "<tempFileName>"))

Two specialized method were added, to test both scala 3.x.x and 2.13.x syntax (and toolkit artifact)

Solves #50.

[TonioGela]

@armanbilge at the moment, the jvm tests are called even in the js and native jobs. I fear this might cause flakiness regarding the artifact that will get published, and if maybe this will solve

lazy val root = tlCrossRootProject
  .aggregate(toolkit, toolkitTest)
  .configureJVM(
    _.settings(
      (Test / test) := (Test / test)
        .dependsOn(toolkitTesting / Test / test)
        .value
    )
  )

WDYT?

[armanbilge]

at the moment, the jvm tests are called even in the js and native jobs

They are? I don't think so.

Edit: so they are. but why 🤔

[TonioGela]

A thing to note is that ScalaCli.main calls System.exit(1) when it fails to compile, so the first broken test will fail fast for everything else. CI will still work and report its broken state, but we won't get a fancy output about the tests that are actually broken.

[armanbilge]

Hmmm, that's actually quite annoying. What if we invoked scala-cli as an external process?

[armanbilge]

It doesn't have to be this PR, but eventually I'd like to publish the JS and Native toolkits and test those as well.

Specifically, we seem to have an issue sometimes where the toolkit requires JS or Native versions newer than what the latest scala-cli is shipping by default, I wonder if we should try to avoid merging those ...

• Compiling for scala-native throws scala.MatchError #49

[TonioGela]

Hmm, this can be done easily, but using scala-cli as an external process, a thing that will solve the System.exit(1) too.

That's another point in favor of this solution 🤔

[TonioGela]

Hmmm, that's actually quite annoying. What if we invoked scala-cli as an external process?

We could do that but I appreciate the leanness of the way is implemented now. I think another viable alternative might be to invoke the inner compile command internally and handle eventual Left, None, Failure or exceptions.

[armanbilge]

think another viable alternative might be to invoke the inner compile command internally

Ah that's the other thing: I don't want to specialize to compile, I think it would be good if we actually try to run the scripts. Otherwise we won't pick up on issues like #105 (comment).

I know it's not as lean, but I think an external process is the way to go long-term.

[TonioGela]

but I think an external process is the way to go long-term.

Hmm, let me think, what do we need apart calling the external process? Will adding VirtusLab/scala-cli-setup@main in the ci.yml be enough?

[armanbilge]

We don't need to install anything, we already have scala-cli on the classpath since we added it as a dependency. We can load the classpath into the buildinfo and use that to invoke scala-cli as an ordinary java app.

[armanbilge]

Here's how we do something similar in Cats Effect for running IOApp tests.

https://github.com/typelevel/cats-effect/blob/a6c5b155be3b729af49eb26204e101f2d527fa80/ioapp-tests/src/test/scala/IOAppSpec.scala#L58-L69

[TonioGela]

Here's how we do something similar in Cats Effect for running IOApp tests.

https://github.com/typelevel/cats-effect/blob/a6c5b155be3b729af49eb26204e101f2d527fa80/ioapp-tests/src/test/scala/IOAppSpec.scala#L58-L69

This definitively requires a night of sleep before trying to be understood. I'll take a look at it tomorrow, thanks ;)

[TonioGela]

I like the idea; it will require a bit of trial and error, but it's feasible. I'll bring this PR back to Draft to make some tests. I have a question, though: why the full classpath was "injected" as a Java property while the platform was injected using BuildInfo?

[TonioGela]

I have a question, though: why the full classpath was "injected" as a Java property while the platform was injected using BuildInfo?

Because a setting cannot depend on a task ofc

[TonioGela]

@armanbilge wdyt of the last commit? It's probably a bit overkilled, but it seems to work. Now it's the case to test both compilation and running for all the other platforms.

One thing I'm not 100% sure is how to structure the js and native tests: maybe it's the case to restore something like Test / test := (Test / test).dependsOn(toolkit.jvm / publishLocal).value, to let the test run after every platform has published locally its own artifact?

[armanbilge]

Instead of this, we can just pass the classpath as a BuildInfo key. Then we also don't need to fork.

Also Compile / dependencyClasspath should be sufficient if we can move scala-cli out of test scope.

[TonioGela]

You could do this only if Compile / dependencyClasspath is a setting. You can't depend on tasks, neither using .value in settings definition. I'll try but I'm not sure.

[armanbilge]

Ahhh. Well that's annoying, forgot that. Ok, so then maybe we need to pass it as an env variable so that JS and Native can pick it up.

[TonioGela]

😎

    buildInfoKeys +=
      BuildInfoKey.map(Compile / dependencyClasspath) { case (k, v) =>
        (k, v.seq.map(_.data.getAbsolutePath).mkString(File.pathSeparator))
      }

[TonioGela]

This can be used in the CE build too :)

[TonioGela]

@armanbilge we should be done :)

[armanbilge]

This is really great! TIL a bunch of stuff too 🤓 Sorry, I just have way too many nitpicks 😅

[TonioGela]

@armanbilge I've addressed most of the suggestions but:

• I can't get toolkit-test to work at all, even locally. Shouldn't //> using test.toolkit typelevel:latest be enough?
• Running on scala.js seems to fail to find the main class

[armanbilge]

• I can't get toolkit-test to work at all,

What's the error? Are you publishing it locally?

---

• Running on scala.js seems to fail to find the main class

Sorry running what exactly? And what is the exact error? I see the CI failure now.

java.lang.Exception: Non zero exit code (1) for run /tmp/3wmKco/.scala

Something seems wrong with the filename?

[TonioGela]

What's the error? Are you publishing it locally?

Nope, I can't recall how to use the toolkit-test artifact, we haven't written a line on doc on how to use it 😓

[armanbilge]

Nope, I can't recall how to use the toolkit-test artifact,

You don't need to do anything, it's automatically in scope when you use scala-cli test command :)

[TonioGela]

java.lang.Exception: Non zero exit code (1) for run /tmp/3wmKco/.scala

Something seems wrong with the filename?

O_O Is the Files[IO].tmpFile impl that works differently in scala.js? Let me check.

[TonioGela]

Nope, I can't recall how to use the toolkit-test artifact,

You don't need to do anything, it's automatically in scope when you use scala-cli test command :)

Using scala-cli test on this:

//> using toolkit typelevel::latest

import cats.effect.*
import munit.*

class Test extends CatsEffectSuite:
  test("test")(IO.unit)

or on the same but with test.toolkit always returns an error about munit not being found

[armanbilge]

typelevel::latest it should be a single colon.

[TonioGela]

CI's green

[TonioGela]

There's something that I don't understand about the native pipeline, it takes more than 60 seconds to compile the first snippet

It may be an ex-post explanation, but after all, within this timeout, we should:

• start the testing machine
• run java externally that will:
  • download the correct scala native version
  • compile the code
  • run the code

So the task is flaky by definition, as we rely on coursier and the internet speed of maven central (or whomever serves the scala native artifact).

The real question is: how could have taken just 30 seconds to do all of this before?

[armanbilge]

Very nicely done 👏

[TonioGela]

The real question is: how could have taken just 30 seconds to do all of this before?

Some of the CI runs that passed before mangling the general timeout took more than 30 seconds O_O'
Is 38 seconds still less than 30 seconds?

[armanbilge]

The 30 second timeout applies per-test, not per-suite. So maybe that's how? or was a single test taking 38 seconds?

[TonioGela]

The 30 second timeout applies per-test, not per-suite. So maybe that's how? or was a single test taking 38 seconds?

Single test apparently: https://github.com/typelevel/toolkit/actions/runs/6309440614/job/17129364102

[TonioGela]

Still, that's not a mistery that we'll solve here @armanbilge :D

Whenever you want,sir