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

Create /tests folder, remove canvas polyfill #109

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

lindapaiste
Copy link
Contributor

Fixes #97
Implements @gohai's recommendations in #82 (comment)

Changes:

  • Move all test-related code to a new folder /tests.
    • Move the /coverage folder.
    • Move the jest configuration file (jest.config.js).
      • Specify the location in package.json since it is no longer at the root.
      • Adjust the paths in the config using <rootDir> syntax.
    • Use jest only on the /unit folder.
    • Add an /integration folder for tests requiring a browser. These are not run at the moment and will need to be set up in a future PR.
  • Uninstall canvas package - fixes Error installing dependencies #97
    • Remove the polyfill from the setupTests.js file.
    • Move files which involve reading an image into the /integration folder.
  • Delete Babel config files and put the config in package.json.

@shiffman shiffman requested a review from gohai March 24, 2024 00:38
@shiffman
Copy link
Member

This looks great, excited to merge! @gohai would have time to review as a second pair of eyes?

@shiffman
Copy link
Member

@ziyuan-linn and I are reviewing this together and the tests run on my machine but not his... I'm going to merge this but we will continue to investigate.

I'll note that I had to run npm rebuild @tensorflow/tfjs-node --build-addon-from-source for the tests to work.

@shiffman shiffman merged commit 98b066c into ml5js:main Mar 26, 2024
@lindapaiste lindapaiste deleted the chore/remove-canvas branch March 26, 2024 22:34
@gohai
Copy link
Member

gohai commented Mar 29, 2024

I am getting an error doing yarn install (not sure if this the same issue @ziyuan-linn had?):

error .../ml5-ng/node_modules/@tensorflow/tfjs-node: Command failed.

From the detailed error message, I am getting the sense that it might be related to me not having Xcode installed? (see also) (I do have the command-line build tools, though)

  File "/Users/gohai/.nvm/versions/node/v18.19.0/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/xcode_emulation.py", line 1737, in _GetXcodeEnv
    "XCODE_VERSION_ACTUAL": XcodeVersion()[0],
                            ^^^^^^^^^^^^^^
  File "/Users/gohai/.nvm/versions/node/v18.19.0/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/xcode_emulation.py", line 1507, in XcodeVersion
    version = CLTVersion()  # macOS Catalina returns 11.0.0.0.1.1567737322
              ^^^^^^^^^^^^
  File "/Users/gohai/.nvm/versions/node/v18.19.0/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/xcode_emulation.py", line 1545, in CLTVersion
    return re.search(regex, output).groupdict()["version"]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'groupdict'

@lindapaiste Since probably a lot of people will run into this issue: what's your thought of moving @tensorflow/tfjs-node into optionalDependencies? When I do this, I am still seeing that installing tfjs-node failed (the first time running yarn install):

warning Error running install script for optional dependency: ".../ml5-ng/node_modules/@tensorflow/tfjs-node: Command failed.

.. but the installation process continues with:

$ patch-package
patch-package 8.0.0
Applying patches...
@tensorflow-models/face-landmarks-detection@1.0.5 ✔
✨  Done in 46.85s.

@shiffman
Copy link
Member

@gohai just curious does running this command fix the error?

npm rebuild @tensorflow/tfjs-node --build-addon-from-source

@ziyuan-linn
Copy link
Member

@gohai I'm not sure if this is the same issue, but removing any spaces in the directory names containing the project fixed the issue for me.

@gohai
Copy link
Member

gohai commented Mar 30, 2024

I filed an issue with tfjs here. I got the same error when I ran npm rebuild @tensorflow/tfjs-node --build-addon-from-source, and the path also doesn't have any spaces. So temperamental!

In the Readme there is a mention that Python 2.7 is needed - hoping it's not that, which would make it very difficult for people to get going..

@lindapaiste
Copy link
Contributor Author

We could consider removing @tensorflow/tfjs-node from the dev dependencies and running the tests using the main/browser version. We will get warnings in the console about how we should be using the Node version, but I get a lot of those still even when I try to use the Node version.

@gohai I definitely appreciate you calling out the difficult dependencies. This is another situation where I've previously installed that package on my computer so it installs easily for me and I forget that there was some complicated setup required at some point in the past.

@gohai
Copy link
Member

gohai commented Apr 3, 2024

@lindapaiste If using the main/browser version is an option, then I think we should go with that. (If there are some additional, more intricate setup steps needed, perhaps we could put it into a tests/Readme.md?)

gohai added a commit that referenced this pull request Jun 4, 2024
This is causing issues during install on machines (that don't have Xcode installed?). As @lindapaiste wrote in #109: We could consider (..) running the tests using the main/browser version. We will get warnings in the console about how we should be using the Node version, but I get a lot of those still even when I try to use the Node version.
shiffman pushed a commit that referenced this pull request Jun 13, 2024
This is causing issues during install on machines (that don't have Xcode installed?). As @lindapaiste wrote in #109: We could consider (..) running the tests using the main/browser version. We will get warnings in the console about how we should be using the Node version, but I get a lot of those still even when I try to use the Node version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error installing dependencies
4 participants