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

Add important build step about packages.json #24

Closed
wants to merge 1 commit into from

Conversation

davidschreiber
Copy link

This step is necessary or tests won't build. The original issue that fixed this issue in all Cordova projects is was only closed recently, but the information was missing in this repository: https://issues.apache.org/jira/browse/CB-12685

This step is necessary or tests won't build. The original issue that fixed this issue in all cordova projects is was only closed recently, but the information was missing in this repository: https://issues.apache.org/jira/browse/CB-12685
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Left some feedback.

@@ -63,6 +64,8 @@ For example, the `cordova-plugin-device` plugin has this nested [`plugin.xml`](h

The `cordova-plugin-test-framework` plugin will automatically find all `tests` modules across all plugins for which the nested tests plugin is installed.

> **Important:** Inside your project's `tests/` folder you also have to create a `package.json`. See the [`package.json` of the `cordova-plugin-device`](https://github.com/apache/cordova-plugin-device/blob/736c7b9dfdfa25a924ffc3d1e409450633a8c00f/tests/package.json) for an example.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, good point. Could I suggest, however, that we reword this a bit and move the paragraph up above the previous "...will automatically find all tests modules" paragraph?

In particular, I would structure this paragraph just like the first paragraph in the section, in a similar, instruction-providing way. Explaining why this needs to be done would be a nice touch, too - it would help users understand the intentions behind each requirement. In particular, the latest version of the tools ensure to run npm install on any plugin added to a project, to ensure to pull in any dependencies. Therefore, plugin authors can now put npm dependencies around their tests into the package.json file.

@filmaj
Copy link
Contributor

filmaj commented May 23, 2017

Ooo, also, could this PR be issued against the master branch? Next time we do a patch release, we will ensure to pull this commit from master into the release branch anyways.

@audreyso
Copy link
Contributor

@filmaj Hi! Has this issue been resolved? May I close it?

@davidschreiber
Copy link
Author

davidschreiber commented Sep 26, 2017

I think this PR is stale by now. I had no time to finish it back then. Also, I can't say if tests still require a package.json and if this somewhere documented. I'd suggest you ask the project maintainers, and maybe file an issue if appropriate. Closing this PR.

@filmaj
Copy link
Contributor

filmaj commented Sep 26, 2017

Ping @stevengill - we might want to still tackle the issue here.

@stevengill
Copy link
Contributor

Yup. This is still good info to add to the Readme. Maybe @audreyso can create a new PR adding the info (low priority)

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.

None yet

4 participants