Skip to content

Fix #17: Fail run in NodeJSEnv if ESModule fails #18

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 22, 2022
Merged

Fix #17: Fail run in NodeJSEnv if ESModule fails #18

merged 4 commits into from
Jan 22, 2022

Conversation

gzm0
Copy link
Contributor

@gzm0 gzm0 commented Jan 17, 2022

No description provided.

@gzm0 gzm0 requested a review from sjrd January 17, 2022 19:49
@gzm0
Copy link
Contributor Author

gzm0 commented Jan 17, 2022

@sjrd does this need a minor version bump? From a JSEnvSuite POV, probably yes, from a NodeJSEnv POV no...

@gzm0 gzm0 force-pushed the fix-es-module-errors branch from 6e844f2 to 4a0110d Compare January 17, 2022 19:52
@gzm0 gzm0 linked an issue Jan 17, 2022 that may be closed by this pull request
NodeJSEnv run does not fail if an ESModule fails #17
Closed
@gzm0 gzm0 mentioned this pull request Jan 17, 2022
Merged
@gzm0
Copy link
Contributor Author

gzm0 commented Jan 17, 2022

Hrm.. Mima fails on the test classes. Not sure what to think about that. Shall we just exclude them?

sjrd
sjrd requested changes Jan 18, 2022
View reviewed changes
js-envs-test-kit/src/main/scala/org/scalajs/jsenv/test/JSEnvSuite.scala Outdated
Comment on lines 72 to 74
val withComValues =
if (config.supportsCom) List(TRUE, FALSE)
else List(TRUE, FALSE)
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this should be

Suggested change
val withComValues =
if (config.supportsCom) List(TRUE, FALSE)
else List(TRUE, FALSE)
val withComValues =
if (config.supportsCom) List(TRUE, FALSE)
else List(FALSE)

js-envs-test-kit/src/main/scala/org/scalajs/jsenv/test/JSEnvSuite.scala Outdated
Comment on lines 87 to 88
if (config.supportsTimeout)
runners.add(r[TimeoutComTests](config))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (config.supportsTimeout)
runners.add(r[TimeoutComTests](config))
if (config.supportsTimeout && config.supportsCom)
runners.add(r[TimeoutComTests](config))

I think?

js-envs-test-kit/src/main/scala/org/scalajs/jsenv/test/JSEnvSuite.scala Outdated
if (config.supportsTimeout)
runners.add(r[TimeoutComTests](config))
if (config.supportsTimeout) {
for (inputKind <- inputKindValues)
Copy link
Member

Choose a reason for hiding this comment

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

This one appears in every case. Have you considered surrounding the entire thing with a single for (inputKind <- inputKindValues)?

js-envs-test-kit/src/main/scala/org/scalajs/jsenv/test/kit/TestKit.scala Outdated
@@ -41,15 +41,17 @@ import org.scalajs.jsenv._
* }}}
*
* @note Methods in [[TestKit]] allow to take a string instead of an [[Input]].
* The string is converted into an input form supported by the [[JSEnv]] to
* execute the code therein.
* The string is converted into an input via `defaultInput`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The string is converted into an input via `defaultInput`.
* The string is converted into an input via `defaultInputKind`.

js-envs-test-kit/src/main/scala/org/scalajs/jsenv/test/kit/TestKit.scala Outdated

private def codeToInput(code: String): Seq[Input] = {
val p = Files.write(
Jimfs.newFileSystem().getPath("testScript.js"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Jimfs.newFileSystem().getPath("testScript.js"),
Jimfs.newFileSystem().getPath("test.js"),

I guess it doesn't make sense to kill this testScript.js anymore.

@sjrd
Copy link
Member

sjrd commented Jan 18, 2022

Hrm.. Mima fails on the test classes. Not sure what to think about that. Shall we just exclude them?

IIUC they're all private[test] classes, right? If yes, we just exclude them.

@gzm0 gzm0 force-pushed the fix-es-module-errors branch 2 times, most recently from 82089d8 to 855a7eb Compare January 18, 2022 20:50
@gzm0
Copy link
Contributor Author

gzm0 commented Jan 18, 2022

I guess I was very tired when I sent this 🤷

I think I have addressed all the comments. The question RE minor version bump still stands.

sjrd
sjrd requested changes Jan 19, 2022
View reviewed changes
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

I think it deserves a minor version bump, yes. The test kit is part of the public API of this repo. Also I don't think there's any reason that a minor version bump for the JSEnv part would cause any trouble.

build.sbt Outdated
Comment on lines 140 to 141
ProblemFilters.exclude[Problem]("org.scalajs.jsenv.test.TimeoutComTests.*")
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ProblemFilters.exclude[Problem]("org.scalajs.jsenv.test.TimeoutComTests.*")
)
ProblemFilters.exclude[Problem]("org.scalajs.jsenv.test.TimeoutComTests.*"),
),

@gzm0 gzm0 force-pushed the fix-es-module-errors branch from 855a7eb to 5ea4d40 Compare January 22, 2022 11:41
@gzm0 gzm0 requested a review from sjrd January 22, 2022 11:41
@sjrd sjrd merged commit eab6373 into scala-js:main Jan 22, 2022
@gzm0 gzm0 deleted the fix-es-module-errors branch January 22, 2022 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrd sjrd sjrd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NodeJSEnv run does not fail if an ESModule fails
2 participants
@gzm0 @sjrd