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

feat: add option to choose the script to run to test dependent #882

Conversation

oliveriosousa
Copy link
Contributor

@oliveriosousa oliveriosousa commented Aug 24, 2021

  • Adds an option to test dependent to choose the script to run (default -> test)

@oliveriosousa oliveriosousa changed the title feat: add script name option to test dependent feat: add option to choose the script to run to test dependent Aug 24, 2021
@achingbrain
Copy link
Member

achingbrain commented Aug 24, 2021

This is necessary because the repos in the ipfs-examples org use an npm script named 'test:example' instead of just 'test', right?

I'm not against this change, but why not just use 'test'?

@oliveriosousa
Copy link
Contributor Author

oliveriosousa commented Aug 24, 2021

Hi @achingbrain,

Yes, some examples, e.g.: react, vue, nextjs, have their proper test scripts. If someone wants to use the standalone repo from, for example, vue, to create an app the only step the user needs to "clean" the repo is removing the test:example and the test folder

@achingbrain
Copy link
Member

I think they'll need to do a bit more as it stands so I'm not sure it's that simple. For example playwright is a dev dep of these projects that would need removing too, plus any other dev deps used by the existing test scripts.

@achingbrain
Copy link
Member

achingbrain commented Aug 24, 2021

If the idea is that you'd use the react, vue etc testing tools after forking the example, the example should really use those tools in the first place so the user has nothing to remove or clean up.

Since they don't currently, I'd propose that as a further work item, rename 'test:example' to 'test' in the examples and make the suggested changes to ipfs/js-ipfs#3821 so we can get it merged and come back to the testing framework problem.

@oliveriosousa
Copy link
Contributor Author

@achingbrain

I've already made the suggested changes, I'm just waiting for this PR ipfs-examples/js-ipfs-examples#26 to have the test script in every example

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM.

Can you please add a test to ensure there are no regressions around this feature, then this is ready to go.

@oliveriosousa
Copy link
Contributor Author

Will do, thanks @achingbrain

test/dependants.js Outdated Show resolved Hide resolved
test/dependants.js Outdated Show resolved Hide resolved
@achingbrain achingbrain merged commit 46392fa into ipfs:master Sep 30, 2021
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.

3 participants