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.

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

fix: Run wasm nodejs tests with no fails #2387

Merged
merged 6 commits into from
Aug 28, 2023
Merged

fix: Run wasm nodejs tests with no fails #2387

merged 6 commits into from
Aug 28, 2023

Conversation

dmvict
Copy link
Contributor

@dmvict dmvict commented Aug 21, 2023

Description

Fix of testing process of crate wasm

Problem*

The nodejs tests of crate wasm fail because mocha cannot resolve some imports. Mocha throws error

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for [filepath]

Summary*

The bug is fixed using alternative import and changed resolving of filepaths.

Documentation

  • I think the crate wasm should provide the documentation for building and testing processes.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@dmvict dmvict changed the title fix: Run mocha nodejs tests with no fails fix: Run wasm nodejs tests with no fails Aug 21, 2023
@kevaundray
Copy link
Contributor

Can you add a test:node command to the package.json?

@dmvict
Copy link
Contributor Author

dmvict commented Aug 22, 2023

Hello @kevaundray

Can you add a test:node command to the package.json?

Yes, I will.

I have a few thoughts about changes. I will try to improve the PR.

@dmvict dmvict marked this pull request as draft August 22, 2023 05:29
@dmvict
Copy link
Contributor Author

dmvict commented Aug 22, 2023 •
edited
Loading

Hello @kevaundray
I've added the command and I've tested my ideas.

I have had two ideas how to make minimal changes. The minimal changes is the changed file index.test.ts and some command/config.
First idea is to setup alternative mocha options and compile the tests. I've tried many different combinations and each time tests was failed.
The second idea is to compile module using standard typescript compiler tsc and then run tests. It works as expected. To run the tests I need to execute two commands:

tsc tests/node/index.test.ts
npm test

The solution requires no additional dependencies and only change the commands in package.json:

+    "build:node": "tsc test/node/index.test.ts",
...
+    "test": "npm run build:node && env TS_NODE_COMPILER_OPTIONS='{\"module\": \"commonjs\" }' mocha",

The command npm test will save artifacts with .js extension. The artifacts can be cleaned after testing.

I think that the second solution is a bit better than the current realization. What do you think?

@kevaundray
Copy link
Contributor

Hello @kevaundray I've added the command and I've tested my ideas.

I have had two ideas how to make minimal changes. The minimal changes is the changed file index.test.ts and some command/config. First idea is to setup alternative mocha options and compile the tests. I've tried many different combinations and each time tests was failed. The second idea is to compile module using standard typescript compiler tsc and then run tests. It works as expected. To run the tests I need to execute two commands:

tsc tests/node/index.test.ts
npm test

The solution requires no additional dependencies and only change the commands in package.json:

+    "build:node": "tsc test/node/index.test.ts",
...
+    "test": "npm run build:node && env TS_NODE_COMPILER_OPTIONS='{\"module\": \"commonjs\" }' mocha",

The command npm test will save artifacts with .js extension. The artifacts can be cleaned after testing.

I think that the second solution is a bit better than the current realization. What do you think?

Second option sounds better -- also this seems like the logical thing to do

@dmvict
Copy link
Contributor Author

dmvict commented Aug 22, 2023

Second option sounds better -- also this seems like the logical thing to do

I will switch to the package and add types to it.

@dmvict dmvict mentioned this pull request Aug 22, 2023
Merged
2 tasks
@dmvict dmvict marked this pull request as ready for review August 22, 2023 13:26
@dmvict dmvict requested a review from TomAFrench August 24, 2023 08:55
@kevaundray kevaundray reopened this Aug 24, 2023
@kevaundray
Copy link
Contributor

btw seems we are only testing browser here:

noir/.github/workflows/wasm.yml

Line 202 in 182d017

run: yarn test:browser

@dmvict
Copy link
Contributor Author

dmvict commented Aug 25, 2023

btw seems we are only testing browser here:

noir/.github/workflows/wasm.yml

Line 202 in 182d017

run: yarn test:browser

I'm trying to keep PRs with minimal changes.

I've added the node testing in the commit.

@kevaundray kevaundray added this pull request to the merge queue Aug 28, 2023
Merged via the queue into noir-lang:master with commit 67b6710 Aug 28, 2023
@kevaundray kevaundray mentioned this pull request Aug 28, 2023
Merged
TomAFrench added a commit that referenced this pull request Aug 28, 2023
@TomAFrench
Merge branch 'master' into deduplicate-instructions
b06cb16 
* master:
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
TomAFrench added a commit that referenced this pull request Aug 28, 2023
@TomAFrench
Merge branch 'master' into assert-eq-builtin
78a57fc 
* master:
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
@Savio-Sou
Copy link
Collaborator

Does this closes #2326?

cc @kobyhallx

@dmvict
Copy link
Contributor Author

dmvict commented Aug 30, 2023

Does this closes #2326?

cc @kobyhallx

Hello @Savio-Sou

Since we have passed nodejs tests https://github.com/noir-lang/noir/actions/runs/6019172378/job/16328784396#step:8:3 we can close the issue.

@Savio-Sou Savio-Sou linked an issue Aug 30, 2023 that may be closed by this pull request
Noir-wasm nodeJs testing #2326
Closed
TomAFrench added a commit that referenced this pull request Aug 30, 2023
@TomAFrench
Merge branch 'master' into kw/bb-binaries
47f2ab6 
* master: (42 commits)
  fix(ssa): Handle right shift with constants (#2481)
  chore(noir): Release 0.10.4 (#2354)
  fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475)
  fix(ssa): Remove padding from ToRadix call with constant inputs (#2479)
  fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463)
  chore: resolve `Instruction` inputs fully before checking against cache (#2472)
  chore: Move independent `run_test` function into nargo core (#2468)
  feat: Standard library functions can now be called with closure args  (#2471)
  feat(frontend): aztec syntactic sugar (feature flagged) (#2403)
  chore(ci): enforce compliance with `cargo fmt` (#2467)
  chore(ci): Allow releases to have additional feature flags (#2405)
  feat: Add `assert_eq` keyword (#2137)
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
  chore: Perform formatting changes to integration tests (#2448)
  ...
TomAFrench added a commit that referenced this pull request Aug 30, 2023
@TomAFrench
Merge branch 'master' into remove-keys-from-artifact
b04ed6b 
* master: (42 commits)
  fix(ssa): Handle right shift with constants (#2481)
  chore(noir): Release 0.10.4 (#2354)
  fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475)
  fix(ssa): Remove padding from ToRadix call with constant inputs (#2479)
  fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463)
  chore: resolve `Instruction` inputs fully before checking against cache (#2472)
  chore: Move independent `run_test` function into nargo core (#2468)
  feat: Standard library functions can now be called with closure args  (#2471)
  feat(frontend): aztec syntactic sugar (feature flagged) (#2403)
  chore(ci): enforce compliance with `cargo fmt` (#2467)
  chore(ci): Allow releases to have additional feature flags (#2405)
  feat: Add `assert_eq` keyword (#2137)
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
  chore: Perform formatting changes to integration tests (#2448)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevaundray kevaundray kevaundray approved these changes

@TomAFrench TomAFrench Awaiting requested review from TomAFrench

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

Successfully merging this pull request may close these issues.

Noir-wasm nodeJs testing
4 participants
@dmvict @kevaundray @Savio-Sou @TomAFrench