Enable/disable webworker test depending on JSEnv #20
Conversation
|
Does munit have a "skipped" status for tests? That would be more semantic than failure. |
|
Good point, they have an ignore tag. I'll look into this. |
|
I guess the reason I'd prefer a failed test here (instead of ignored/skipped) is that if CI passes nobody checks the log (or at least, I don't 😝) in which case this could be easily overlooked. What we could do is ignore/skip for local but fail in CI. |
armanbilge
changed the title
| .foreach { _ => | ||
| test("pass the MacrotaskSuite in a web worker") { | ||
| val p = Promise[Boolean]() | ||
| override def munitIgnore = !Set("Firefox", "Chrome").contains(BuildInfo.jsEnv) |
There was a problem hiding this comment.
@djspiewak how about this?
There was a problem hiding this comment.
Can we put the browser check into the buildInfoKeys calculation instead? Basically turn the build info into something more like "isBrowser" -> .... That does a better job of centralizing things so that we can add/remove browsers more easily
There was a problem hiding this comment.
Good idea. Also this is busted right now anyway lol.
| .foreach { _ => | ||
| test("pass the MacrotaskSuite in a web worker") { | ||
| val p = Promise[Boolean]() | ||
| override def munitIgnore = !Set("Firefox", "Chrome").contains(BuildInfo.jsEnv) |
There was a problem hiding this comment.
Can we put the browser check into the buildInfoKeys calculation instead? Basically turn the build info into something more like "isBrowser" -> .... That does a better job of centralizing things so that we can add/remove browsers more easily
| addCommandAlias("ciNode", "; set Global / useJSEnv := JSEnv.NodeJS; test; core/doc") | ||
| addCommandAlias("ciFirefox", "; set Global / useJSEnv := JSEnv.Firefox; test; set Global / useJSEnv := JSEnv.NodeJS") | ||
| addCommandAlias("ciChrome", "; set Global / useJSEnv := JSEnv.Chrome; test; set Global / useJSEnv := JSEnv.NodeJS") | ||
| addCommandAlias("ciJSDOMNodeJS", "; set Global / useJSEnv := JSEnv.JSDOMNodeJS; test; set Global / useJSEnv := JSEnv.NodeJS") |
There was a problem hiding this comment.
This is how it was configured in CE. Just an omission, or is Global a weird/bad way to do things?
There was a problem hiding this comment.
Ah yeah. Global is actually fine and it is the correct thing to do here.
Since in CI we're only running the webworker tests in the appropriate environment anyway, we don't need this programmatic check for webworker support to decide whether to enable/disable the test. Instead, better the test fails rather than be silently ignored.
OTOH, this would break
root/test. Thoughts?