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

Updated documentaion #1160

Merged
merged 3 commits into from
May 18, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 21 additions & 35 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,24 +1,32 @@
# Contributing to Prebid.js
Contributions are always welcome. To contribute, [fork](https://help.github.com/articles/fork-a-repo/) Prebid.js, commit your changes, and [open a pull request](https://help.github.com/articles/using-pull-requests/).
Contributions are always welcome. To contribute, [fork](https://help.github.com/articles/fork-a-repo/) Prebid.js, commit your changes, and [open a pull request](https://help.github.com/articles/using-pull-requests/) against the master branch.

## Pull Requests
Please make sure that pull requests are scoped to one change, and that any added or changed code includes tests with greater than 80% code coverage. See [Testing Prebid.js](#testing-prebidjs) for help on writing tests.
Details about the Pull Request process can be found [here](./pr_review.md).
Copy link
Member

Choose a reason for hiding this comment

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

hate to have duplicative information, but I feel like stating 80% code coverage is nice to have here, even if it is in the linked doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


## Issues
[prebid.org](http://prebid.org/) contains documentation that may help answer questions you have about using Prebid.js. If you can't find the answer there, try searching for a similar issue on the [issues page](https://github.com/prebid/Prebid.js/issues). If you don't find an answer there, [open a new issue](https://github.com/prebid/Prebid.js/issues/new).
[prebid.org](http://prebid.org/) contains documentation that may help answer questions you have about using Prebid.js.
If you can't find the answer there, try searching for a similar issue on the [issues page](https://github.com/prebid/Prebid.js/issues).
If you don't find an answer there, [open a new issue](https://github.com/prebid/Prebid.js/issues/new).

## Documentation
If you have a documentation issue or pull request, please open a ticket or PR in the [documentation repository](https://github.com/prebid/prebid.github.io).

## Testing Prebid.js
Pull requests to the Prebid.js library will need to include tests with greater than 80% code coverage for any changed/added code before they can be merged into master.
## Writing Tests
Tests are stored in the [test/spec](test/spec) directory. Tests for Adapters are located in [test/spec/adapters](test/spec/adapters).
They can be run with the following commands:

This section describes how to test code in the Prebid.js repository to help prepare your pull request.
- `gulp test` - run the test suite once (`npm test` is aliased to call `gulp test`)
- `gulp serve` - run the test suite once, but re-run it whenever a file in the `src` or `test` directory is modified

### Writing tests
Before your Pull Request will be considered for merge:

- All new and existing tests must pass
- Added or modified code must have greater than 80% coverage. The coverage report will be generated in `build/coverage/lcov/lcov-report/index.html`

### Test Guidelines
When you are adding code to Prebid.js, or modifying code that isn't covered by an existing test, test the code according to these guidelines:

- If the module you are working on is already partially tested by a file within the `test` directory, add tests to that file
- If the module you are working on is already partially tested by a file within the `test/spec` directory, add tests to that file
- If the module does not have any tests, create a new test file
- Group tests in a `describe` block
- Test individual units of code within an `it` block
Expand All @@ -36,31 +44,8 @@ When you are adding code to Prebid.js, or modifying code that isn't covered by a
- If you need to check `adloader.loadScript` in a test, use a `stub` rather than a `spy`. `spy`s trigger a network call which can result in a `script error` and cause unrelated unit tests to fail. `stub`s will let you gather information about the `adloader.loadScript` call without affecting external resources
- When writing tests you may use ES2015 syntax if desired

### Running tests
After checking out the Prebid.js repository and installing dev dependencies with `npm install`, use the following commands to run tests as you are working on code:

- `gulp test` will run the test suite once (`npm test` is aliased to call `gulp test`)
- `gulp serve` will run tests once and stay open, re-running tests whenever a file in the `src` or `test` directory is modified

### Checking results and code coverage
Check the test results using these guidelines:

- Look at the total number of tests run, passed, and failed in the shell window.
- If all tests are passing, great.
- Otherwise look for errors printed in the console for a description of the failing test.
- You may need to iterate on your code or tests until all tests are passing.
- Make sure existing tests still pass.
- There is a table below the testing report that shows code coverage percentage, for each file under the `src` directory.
- Each time you run tests, a code coverage report is generated in `build/coverage/lcov/lcov-report/index.html`.
Copy link
Member

Choose a reason for hiding this comment

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

Think having a link to this report is good to preserve, maybe just add a heading for Test Coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. It's included under "Writing Tests" though.

My thought process was basically "the only time I care about code coverage is when I check to make sure my tests are good anyway."

- This is a static HTML page that you can load in your browser.
- On that page, navigate to the file you are testing to see which lines are being tested.
- Red indicates that a line isn't covered by a test.
- Gray indicates a line that doesn't need coverage, such as a comment or blank line.
- Green indicates a line that is covered by tests.
- The code you have added or modified must have greater than 80% coverage to be accepted.

### Examples
Prebid.js already has lots of tests. Read them to see how Prebid.js is tested, and for inspiration:
### Test Examples
Prebid.js already has many tests. Read them to see how Prebid.js is tested, and for inspiration:

- Look in `test/spec` and its subdirectories
- Tests for bidder adaptors are located in `test/spec/adapters`
Expand All @@ -87,5 +72,6 @@ describe('<Adapter>', () => {
The Prebid.js testing stack contains some of the following tools. It may be helpful to consult their documentation during the testing process.

- [Mocha - test framework](http://mochajs.org/)
- [Karma - test runner](https://karma-runner.github.io/1.0/index.html)
- [Chai - BDD/TDD assertion library](http://chaijs.com/)
- [Sinon - spy, stub, and mock library](http://sinonjs.org/)
- [Sinon - spy, stub, and mock library](http://sinonjs.org/)
12 changes: 9 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ This README is for developers who want to contribute to Prebid.js. For user-fac
$ cd Prebid.js
$ yarn install

Prebid now supports the `yarn` npm client. This is an alternative to using `npm` for package management, though `npm` will continue to work as before.
Prebid also supports the `yarn` npm client. This is an alternative to using `npm` for package management, though `npm` will continue to work as before.

For more info about yarn see https://yarnpkg.com
For more info, see [the Yarn documentation](https://yarnpkg.com).

<a name="Build"></a>

## Build for Dev
## Build for Development

To build the project on your local machine, run:

Expand Down Expand Up @@ -148,6 +148,8 @@ A watch is also in place that will run continuous tests in the terminal as you e

Many SSPs, bidders, and publishers have contributed to this project. [60+ Bidders](https://github.com/prebid/Prebid.js/tree/master/src/adapters) are supported by Prebid.js.

For guidelines, see [Contributing](./CONTRIBUTING.md).

Our PR review process can be found [here](https://github.com/prebid/Prebid.js/tree/master/pr_review.md).

### Add a Bidder Adapter
Expand Down Expand Up @@ -192,3 +194,7 @@ Prebid.js is supported on IE9+ and modern browsers.

### Governance
Review our governance model [here](https://github.com/prebid/Prebid.js/tree/master/governance.md).

### Additional Resources

- [Prebid Examples](http://prebid.org/dev-docs/getting-started.html)
Copy link
Member

Choose a reason for hiding this comment

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

I think this link belongs up near the top of the readme - since the consumer is anyone that is trying to implement prebid.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah... I agree, this does make much more sense