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

chore: add ava config and some basic tests #215

Merged
merged 13 commits into from
Jan 5, 2023
Merged

chore: add ava config and some basic tests #215

merged 13 commits into from
Jan 5, 2023

Conversation

yusefnapora
Copy link
Contributor

This needs some cleanup, but I got ava + typescript to work after a bit of wrestling.

So far I've only added some simple tests to keyring-core that don't depend on the service, but I've also started on a mock access service so we can test registerSpace, etc.

I had to add "type": "module" to the keyring-core package.json to get the ava + typescript setup to work. That seems pretty safe, since we're only including the rollup bundle in the published package, so it shouldn't make a difference to users.

It seems like the mock services would be useful for all the packages, so maybe we should make a shared test-utils package?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 5, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 75b9e3d:

Sandbox Source
@w3ui/example-react-file-upload Configuration
@w3ui/example-react-sign-up-in Configuration
@w3ui/example-react-uploads-list Configuration
@w3ui/example-solid-file-upload Configuration
@w3ui/example-solid-sign-up-in Configuration
@w3ui/example-solid-uploads-list Configuration
@w3ui/example-vue-file-upload Configuration
@w3ui/example-vue-sign-up-in Configuration
@w3ui/example-vue-uploads-list Configuration

Copy link
Member

@travis travis left a comment

Choose a reason for hiding this comment

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

this is fantastic!! I left a couple comments but already pushed fixes for them - happy to back either of my changes out if you don't want them, but I was in the code testing the --experimental-specifier-resolution change and it was easy to knock them both out

packages/keyring-core/test/utils/mock-service.ts Outdated Show resolved Hide resolved
packages/keyring-core/test/agent.ts Outdated Show resolved Hide resolved
@travis
Copy link
Member

travis commented Jan 5, 2023

this is already strictly better than what we have now so I'd be happy if you merged as-is!

It seems like the mock services would be useful for all the packages, so maybe we should make a shared test-utils package?

I think it might be a little cleaner for

  1. each package to export useful test utils for the functionality it provides and
  2. to introduce a services package of some sort per proposal: add services-core and react-services packages to encapsulate upload/access service configuration #156 where we can implement service Providers and land these mock services -

I'm worried that a shared test-utils package will end up depending on all these packages, and that all the packages will end up dev-depending on test-utils leading to a gnarly knot of dependency resolution. Strictly speaking I think either will work, but the idea of a dependency knot like this puts a knot in my stomach...

@yusefnapora
Copy link
Contributor Author

awesome, thanks for the fixes @travis :)

I think you're right that the services package is where it's at. Should we just pull the WIP mock service out of this PR, so it's not cluttering up the joint? It's not being used yet (except in the skipped test).

to be reborn in the upcoming services package...
@travis
Copy link
Member

travis commented Jan 5, 2023

Should we just pull the WIP mock service out of this PR

Yep that sounds great - probably time to create a new package (@w3ui/services?) where that can live. If you're up for creating it go for it, otherwise I'm happy to get that into a draft PR in the next day or so.

@travis
Copy link
Member

travis commented Jan 5, 2023

oh also maybe just singular @w3ui/service since @gobengo unified the two services into one in storacha/w3up#325 (comment)

@yusefnapora yusefnapora marked this pull request as ready for review January 5, 2023 23:54
@yusefnapora
Copy link
Contributor Author

sweet, this is passing on CI now that I bumped it up to node 18.

I'll go ahead and merge now that the mocks are gone. @w3ui/service sounds good to me for the new package. I can start on that tomorrow unless you feel like grabbing it sooner :)

@yusefnapora yusefnapora merged commit 4bd233b into main Jan 5, 2023
@yusefnapora yusefnapora deleted the feat/ava branch January 5, 2023 23:57
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.

2 participants