Skip to content

fix node tests in subdirs #807

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

Merged
merged 3 commits into from
Apr 10, 2018
Merged

fix node tests in subdirs #807

merged 3 commits into from
Apr 10, 2018

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented Apr 10, 2018

This was using the basename of the file before, oops!

@jakemac53 jakemac53 added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Apr 10, 2018
@jakemac53 jakemac53 requested a review from natebosch April 10, 2018 21:09
var jsPath =
p.join(precompiledPath, p.basename(testPath) + ".node_test.dart.js");

var jsPath = p.join(precompiledPath, testPath + ".node_test.dart.js");
Copy link
Member

Choose a reason for hiding this comment

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

single quotes for consistency with the line 2 below.

Would also prefer '$testPath.node_test.dart.js'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, in general theres a crazy mix of single/double quotes here so eventually we might want to fix that repo-wide, but at least its consistent within this method now...

@@ -248,7 +246,7 @@ class NodePlatform extends PlatformPlugin
try {
return await Process.start(settings.executable,
settings.arguments.toList()..add(jsPath)..add(socketPort.toString()),
environment: {'NODE_PATH': nodePath});
environment: {'NODE_PATH': nodePath}w);
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol

@@ -248,7 +246,7 @@ class NodePlatform extends PlatformPlugin
try {
return await Process.start(settings.executable,
settings.arguments.toList()..add(jsPath)..add(socketPort.toString()),
environment: {'NODE_PATH': nodePath});
environment: {'NODE_PATH': nodePath}w);

Choose a reason for hiding this comment

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

Curious what w means in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it means my finger slipped ;)

@jakemac53 jakemac53 merged commit 0ff3bf1 into master Apr 10, 2018
@jakemac53 jakemac53 deleted the node-fixes branch April 10, 2018 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants