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

Fixed Incorrect Callback Return in CLI Module Validation #1383

Merged
merged 2 commits into from
Jul 11, 2020

Conversation

torch2424
Copy link
Contributor

relates to DuncanUszkay1#14

Please see the PR above for the context. But TL;DR I was working on closures, and the tests would hang when failing when validating the module. I think this return is meant to happen outside of the measure.

With this fix, tests will not hang whenever a module cannot be validated correctly 😄 👍

cc @dcodeIO @MaxGraey

@torch2424 torch2424 added the bug label Jul 9, 2020
@torch2424 torch2424 requested review from dcodeIO and MaxGraey July 9, 2020 17:52
@torch2424 torch2424 changed the title Fixed Incorrect Callback Return in Module Validation Fixed Incorrect Callback Return in CLI Module Validation Jul 9, 2020
cli/asc.js Outdated
@@ -653,12 +653,16 @@ exports.main = function main(argv, options, callback) {
// Validate the module if requested
if (!args.noValidate) {
stats.validateCount++;
let callbackErrorResponse;
Copy link
Member

@dcodeIO dcodeIO Jul 9, 2020

Choose a reason for hiding this comment

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

Good catch! Perhaps let's make this an isValid var and throw the error / dispose if (!isValid)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me! 😄

Copy link
Contributor Author

@torch2424 torch2424 Jul 9, 2020

Choose a reason for hiding this comment

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

Oh wait, if I'm not mistaken we expect the callback to return a status code?

So it would be something like? 🤔 :

// Validate the module if requested
  if (!args.noValidate) {
    stats.validateCount++;
    let isValid;
    stats.validateTime += measure(() => {
      if (!module.validate()) {
        module.dispose();
        isValid = callback(Error("Validate error")) === 0;
      }
    });
    if (!isValid) {
      return 1;
    }
  }

cc @dcodeIO

Copy link
Member

Choose a reason for hiding this comment

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

Something like the following maybe

if (!args.noValidate) {
  stats.validateCount++;
  let isValid;
  stats.validateTime += measure(() => {
    isValid = module.validate();
  });
  if (!isValid) {
    module.dispose();
    return callback(Error("validate error"));
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Callback there is typically the default, unless one uses the CLI API. Returns 1 when an error is provided. The returned value becomes the exit code of asc.

@torch2424
Copy link
Contributor Author

@dcodeIO Made requested changes! And I am pretty sure they work, as I forced some compiler errors (which I think would break validation), but it didn't hang 😄

Screenshot from 2020-07-10 15-28-18

@dcodeIO dcodeIO merged commit 3f97e1a into AssemblyScript:master Jul 11, 2020
@dcodeIO
Copy link
Member

dcodeIO commented Jul 11, 2020

Great, thanks!

@github-actions
Copy link

🎉 This PR is included in version 0.13.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@torch2424 torch2424 deleted the fix-validate-tests-hang branch July 13, 2020 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants