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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ospec): Allow custom reporters for CI reasons (#2019) #2020

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
2 participants
@zyrolasting
Copy link
Contributor

zyrolasting commented Nov 20, 2017

Valiantly defy one bullet in the ospec goal of blocking test configuration by adding custom reporter support. 馃憫

Motivation and Context

See #2019

Description

I added a reporter param to o.run() to allow custom processing of results, falling back to current behavior if one is not specified.

ospec absolutely should limit test space config, but not having this feature has severely limited my ability to pass results along to a CI system when running ospec in a browser or Node.js. This change in made with the intent to leverage the entire Mithril toolset for real-world systems.

How Has This Been Tested?

I used o.new() to make a new ospec runner, and then used the clone's custom reporter to run assertions against the original ospec instance. So if the reporting did not work, test-ospec.js would fail.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md
@tivac
Copy link
Member

tivac left a comment

Minor nits to fix before we can merge, but generally your approach & the tests look great.

can use custom reporters in `o.run()` to process these results.

```javascript
o.run(function (results) {

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

No space in function (

o.run(function (results) {
// results is an array
results.forEach(function (result) {

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

No space in function (

As an example, the following test's result message will be `"true\nshould equal\nfalse"`.

```javascript
o.spec("message", function () {

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

No space in function (

If you specify an assertion description, that description will appear two lines above the subject.

```javascript
o.spec("message", function () {

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

No space in function (

In the below example, `result.context` would be `testing > rocks`.

```javascript
o.spec("testing", function () {

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

No space in function (


```javascript
o.spec("testing", function () {
o.spec("rocks", function () {

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

No space in function (

---

## Goals

- Do the most common things that the mocha/chai/sinon triad does without having to install 3 different libraries and several dozen dependencies
- Disallow configuration in test-space:
- Disallow ability to pick between API styles (BDD/TDD/Qunit, assert/should/expect, etc)
- Disallow ability to pick between different reporters

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

Let's reword this line & move it to the bottom instead of removing it entirely. Maybe something like

  • Provide a default simple reporter
@@ -149,13 +184,13 @@ o.spec("ospec", function() {
})
})

o.spec('stack trace cleaner', function() {
o('handles line breaks', function() {
o.spec("stack trace cleaner", function() {

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

馃憤鉁忥笍馃懏

@@ -438,7 +438,7 @@ o.spec("xhr", function() {
o("set timeout to xhr instance", function() {
mock.$defineRoutes({
"GET /item": function() {
return {status: 200, responseText: ''}
return {status: 200, responseText: ""}

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

Remove this from the PR please. I like the consistency increase but would rather keep this PR as focused as possible and touching as few files as possible.

This comment has been minimized.

@zyrolasting

zyrolasting Nov 20, 2017

Contributor

Of course. Asking so I can do as the Romans do: Do you all normally amend commits and force push to update PRs in this situation, or just add revert commits?

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

Whatever strikes your fancy, I'm gonna do a squash onto next at the end anyways.

@zyrolasting zyrolasting force-pushed the zyrolasting:ospec-reporter branch 3 times, most recently from 707d639 to a7ae3c4 Nov 20, 2017

@zyrolasting

This comment has been minimized.

Copy link
Contributor

zyrolasting commented Nov 20, 2017

@tivac Requested changes are in. Ready for second review pass.

@zyrolasting zyrolasting force-pushed the zyrolasting:ospec-reporter branch from a7ae3c4 to e12bf2a Nov 20, 2017

@tivac
Copy link
Member

tivac left a comment

Much closer, one small stylistic issue left.

@@ -236,6 +237,13 @@ module.exports = new function init(name) {

function report() {
var status = 0

if (typeof reporter === "function") {

This comment has been minimized.

@tivac

tivac Nov 20, 2017

Member

This should still be a single line to match the rest of the codebase.

This comment has been minimized.

@zyrolasting

zyrolasting Nov 20, 2017

Contributor

Got it in. Thanks for your patience.

@zyrolasting zyrolasting force-pushed the zyrolasting:ospec-reporter branch from e12bf2a to 672d7d1 Nov 20, 2017

@tivac

tivac approved these changes Nov 20, 2017

Copy link
Member

tivac left a comment

馃憤 Thanks for sticking with it!

@tivac tivac merged commit 78eeb2b into MithrilJS:next Nov 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zyrolasting zyrolasting deleted the zyrolasting:ospec-reporter branch Nov 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment