Skip to content

@o1js/testing #2093

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

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

@o1js/testing #2093

wants to merge 51 commits into from

Conversation

boray
Copy link
Member

@boray boray commented Mar 24, 2025

This pull request introduces a new package @o1js/testing and includes several changes to support this addition. The changes involve setting up the package structure, configuration files, and exporting necessary utilities.

New Package Setup:

  • src/testing/package.json: Created a new package.json file to define the @o1js/testing package, including scripts for building and testing, and specifying dependencies.
  • src/testing/CHANGELOG.md: Added a changelog file to document notable changes in the project, adhering to the Keep a Changelog format and Semantic Versioning.

Configuration and Build Tools:

  • src/testing/jest.config.js: Added a Jest configuration file to set up the testing environment for the package, including presets and transform ignore patterns.
  • tsconfig.testing.json: Created a TypeScript configuration file specifically for the testing package to manage compilation settings and include/exclude patterns.
  • src/testing/copy-bindings.js: Added a script to copy bindings from the build directory to the distribution directory.

Exporting Utilities:

  • src/testing/index.ts: Created an index file to export all testing utilities from the src/testing.js file.
  • src/testing/src/testing.ts: Imported and re-exported various utilities from equivalent.js, property.js, and constraint-system.js to make them available as part of the testing package.
  • src/lib/testing/equivalent.ts: Updated the export statement to include AnyTupleFunction.

TODO

This PR will have couple of follow-up tasks and PR's for:

  • Creating namespaces in src/lib/testing for better organization of tools
  • Adding TSDoc style comments to src/lib/testing
  • Discuss creating a packages directory for o1js,mina-signer and @o1js/testing internally.

@boray boray changed the title @o1js/testing prototype @o1js/testing Mar 24, 2025
Copy link
Contributor

@45930 45930 left a comment

Choose a reason for hiding this comment

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

Overall this looks reasonable! Can I try using it locally with npm link? I tried from the src/lib/testing folder and it didn't work.

@boray
Copy link
Member Author

boray commented Apr 3, 2025

Overall this looks reasonable! Can I try using it locally with npm link? I tried from the src/lib/testing folder and it didn't work.

I used yalc to test the package so far. It should work with npm link too. Can you explain how it failed?

@45930
Copy link
Contributor

45930 commented Apr 4, 2025 via email

@boray
Copy link
Member Author

boray commented Apr 4, 2025

I don’t think it’s a big deal either way. But it just seems like anyone installing this package will know how to install o1js too.

oops, I missed it's for o1js not @o1js/testing. Indeed, it's an overkill. I changed this while ago but not commited. It just has a note about peer dependency. The current version of the readme is incomplete. I'll be pushing updates tomorrow.

"version": "0.0.1",
"type": "module",
"scripts": {
"build": "tsc -p ../../tsconfig.testing.json && node copyBindings.js",
Copy link
Member

@Trivo25 Trivo25 Apr 4, 2025

Choose a reason for hiding this comment

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

right now this builds everything in the project, incl. mina-signer, and copies all artefacts into **/testing/dist/** - I think we should be a bit more selective here and only copy what we need

Copy link
Member Author

Choose a reason for hiding this comment

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

Only mina-signer is not used, but I didn't try to prevent it from building since npm run build in o1js also builds mina-signer.

Comment on lines 33 to 47
export const {
constraintSystem,
not,
and,
or,
fulfills,
equals,
contains,
allConstant,
ifNotAllConstant,
isEmpty,
withoutGenerics,
print,
repeat,
} = ConstraintSystem_;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put the constraint system stuff in a separate name space.
most people won't need it, and it's confusing to pollute the name space with many simple functions like and, equals etc

Copy link
Contributor

Choose a reason for hiding this comment

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

similar how Random is exported as its own namespace!

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed but then existing tests would be incompatible* (directly) with the package. What do you think about a new PR that creates such namespace and updates all tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

wdym they "would be incompatible"? I'm asking for a change in this file, testing.ts, not in the existing tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const {
constraintSystem,
not,
and,
or,
fulfills,
equals,
contains,
allConstant,
ifNotAllConstant,
isEmpty,
withoutGenerics,
print,
repeat,
} = ConstraintSystem_;
export { ConstraintSystem_ as ConstraintSystem }

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a deliberate choice to not create a namespace to not change the usage from constraintSystem.fromZkProgram() to ConstraintSystem.constraintSystem.fromZkProgram() to make the package "plug and play" compatible with the existing tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong choice IMO. People won't use this, it pollutes the namespace, and it's still plug and play compatible if you just unpack the CobstraintSystem object at the beginning

Copy link
Member Author

Choose a reason for hiding this comment

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

"it pollutes the namespace" I agree with this but I don't think the latter is a good solution. Why not we fix the pollution in its roots at constraint-system.ts? Similar fix would be needed for equivalent.ts too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, fix it where you think it fits best!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is one of those things where it's better to get it merged and change the namespace as a follow-up PR. I like to follow the "minimally invasive migration" => "improve" flow.

But I do agree that namespaceing would make sense.

A nice stopgap would be to do both (redundantly), have all the docs point to the namespace, and then migrate the old stuff. We can do this before releasing the npm package so that people don't use it/changing the API won't be a breaking change.

Copy link
Contributor

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

I'm going to give this a ✅, but I would prefer (if we all agree) that we move this package to /packages/testing before landing this PR.

@Trivo25
Copy link
Member

Trivo25 commented Apr 8, 2025

I assumed you have the credentials for npm for o1js and you can release on my behalf. No worries, there won't be releases often :)

We shouldn’t depend on a single person who may or may not have the credentials to release a package. For our other releases, we already have access to the credentials via CI, and we should follow the same approach here. Since we're introducing this new package to developers, we can expect feedback and requests for improvements - which means we'll likely need to release more frequently. Relying on one individual creates an unnecessary bottleneck and adds avoidable overhead to what could be a much simpler process.

Copy link

@0x471 0x471 left a comment

Choose a reason for hiding this comment

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

It seems good to merge for now, and the folder structure can be improved in follow-up PRs.

@45930
Copy link
Contributor

45930 commented Apr 8, 2025

We shouldn’t depend on a single person who may or may not have the credentials to release a package. For our other releases, we already have access to the credentials via CI, and we should follow the same approach here.

Yeah, releasing the package automatically if the version is incremented seems best to me. How do we handle mina signer?

@45930
Copy link
Contributor

45930 commented Apr 8, 2025

I'm going to give this a ✅, but I would prefer (if we all agree) that we move this package to /packages/testing before landing this PR.

I think a separate PR is ok because the scope should also include migrating mina-signer (and o1js itself?) to the packages folder. I wouldn't want to muddy this PR with extra diffs and potential issues from that. We can handle migrating all packages to a standardized format in one PR.

@Trivo25
Copy link
Member

Trivo25 commented Apr 8, 2025

How do we handle mina signer?

Same way. We can simply copy the workflow from here

Release-on-NPM:
if: github.ref == 'refs/heads/main'
timeout-minutes: 180
runs-on: ubuntu-latest
needs: [Build-And-Test-Server, Run-Unit-Tests, Build-And-Test-Web]
steps:
- name: Restore repository
uses: actions/cache@v4
with:
path: .
key: repo-${{ github.sha }}
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: '20'
- name: Build o1js
run: |
npm ci
npm run prepublishOnly
- name: Publish to NPM if version has changed
id: publish
uses: JS-DevTools/npm-publish@v3
with:
token: ${{ secrets.NPM_TOKEN }}
strategy: upgrade
env:
INPUT_TOKEN: ${{ secrets.NPM_TOKEN }}
- name: Configure Git
run: |
git config --local user.email "[email protected]"
git config --local user.name "GitHub Action"
- name: Tag new version
if: ${{ steps.publish.outputs.type }} # https://github.com/JS-DevTools/npm-publish?tab=readme-ov-file#action-output
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
RELEASED_VERSION: ${{ steps.publish.outputs.version }}
run: |
git tag $RELEASED_VERSION
git push origin $RELEASED_VERSION
- name: Create Release
uses: ncipollo/release-action@v1
if: success()
with:
token: ${{ secrets.GITHUB_TOKEN }}
tag: ${{ steps.publish.outputs.version }}
generateReleaseNotes: true
name: Release ${{ steps.publish.outputs.version }}
Release-mina-signer-on-NPM:
if: github.ref == 'refs/heads/main'
timeout-minutes: 180
runs-on: ubuntu-latest
needs: [Build-And-Test-Server, Run-Unit-Tests, Build-And-Test-Web]
steps:
- name: Restore repository
uses: actions/cache@v4
with:
path: .
key: repo-${{ github.sha }}
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: '20'
- name: Build mina-signer
run: |
npm ci
cd src/mina-signer
npm ci
npm run prepublishOnly
- name: Publish to NPM if version has changed
uses: JS-DevTools/npm-publish@v3
with:
token: ${{ secrets.NPM_TOKEN }}
package: './src/mina-signer/package.json'
strategy: upgrade
env:
INPUT_TOKEN: ${{ secrets.NPM_TOKEN }}

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

please update the npm-deps-hash

boray and others added 2 commits April 9, 2025 02:30
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@boray boray dismissed github-actions[bot]’s stale review April 8, 2025 23:34

I committed bot's suggestion.

@@ -0,0 +1,22 @@
# Changelog
Copy link
Member

Choose a reason for hiding this comment

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

In a follow up PR we should also organize our github actions better. Right now everything is a bit chaotic

"node": "./dist/node/testing/index.js",
"default": "./dist/node/testing/index.js"
},
"files": [
Copy link
Member

Choose a reason for hiding this comment

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

We are including mina-signer and other internal dependencies in the package when we publish it to npm. While this might not be a big size increase in the overall package (given that the compile artefacts alone are about 45mb), it's still suboptiomal to include files that have nothing to do with the actual library in the released package

Copy link
Member

Choose a reason for hiding this comment

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

If or when we end up splitting everything into seperate package folders, we should definitely clean this up. So let's make sure we keep this in mind, I approve in hopes that we will follow up on this soon

@boray boray requested a review from Copilot April 14, 2025 07:09
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 12 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • npmDepsHash: Language not supported
  • src/testing/package.json: Language not supported
  • tsconfig.testing.json: Language not supported

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.

6 participants