Service tests #937

paulmelnikow · 2017-04-08T17:10:43Z

Goals

Eliminate manual testing which is error-prone and time consuming, and must be repeated many times through the PR review process
Make contributing more fun. For many, fixing bugs and making new badges is faster and more satisfying with automated tests than with manual testing.
Push out the work of testing new badges to a much broader net. The PR originator could write tests, but so could any other contributor who wants to push review along.
Detect badge failures resulting from changes in vendor contracts without waiting for user reports.
Detect and prevent regressions in the code.
Be runnable, readable, writable, and editable by as many developers as possible, including those who may not be familiar with JavaScript test tools.

For further background, I wrote a lot about my thinking in #927.

Features

Perform live tests, which hit the real services.
- Recorded tests would be valuable, especially for preventing regression, but the tooling to maintain the recorded requests (Nock Back) is buggy and needs work.
Easily intercept vendor requests to test code branches which can't be exercised any other way.
Manually run a subset of tests.
In a pull request, show the results of the affected subset of tests.
Periodically run the whole suite.
Use a concise vendor spec which is easy to read, write, and modify.
Help new devs along, so the process is not intimidating.
Easily extended to other testing scenarios, like recorded tests, and smoke tests against the production server.

Solution

Concise vendor spec, implemented as a light wrapper. Most of the work is delegated to three existing tools:
- IcedFrisby, an API-testing tool with concise syntax which registers tests with Mocha
- Joi, which is bundled with IcedFrisby and helps with assertions
- Nock, an HTTP mocking and expectations library
- icedfrisby-nock, which makes it easy to use Nock with IcedFrisby
One example of a vendor test.
A test runner which will run all the tests: npm run test:vendor. It's invoked during scheduled nightly builds in Travis, which means the results are public and easily accessed. Any failure will be reported as such, which seems helpful since it will bring it to our attention. However we could silence them if we find the failures are too noisy. Pushes to master run lint and the existing unit tests. Only the nightly build adds the vendor tests.
A test runner that lets a contributor run just the vendors they want from the CLI: npm run test:vendor -- --only=travis
A neat integration with PRs. I wanted a way to see the results of the affected vendor tests right in the PR, because it would expedite review that much more. I considered a bot that could be triggered from comments. I considered automatically detecting the affected vendors using the diff. But I settled on something much simpler, which also seems rather elegant. The contributor includes a tag like [travis], [github], or [sonar] in the PR title. The CI runner knows which PR it's building and looks up the title, extracts the tag, and runs only those vendors. If the person forgets to tag, they or the maintainer can go back and edit later. The tag is transparent, learnable, and has no new moving parts. As a bonus, it provides useful context to us humans in the PR list.
Tooling to run code coverage locally, to help devs cover all the branches in the vendor they are testing. In the future, this could be reported to Coveralls during the nightly build, to make it easier for contributors to identify untested code paths, and provide a progress scorecard.
Approachable developer docs which provide clear expectations for contributions.

Suggested review order

Entry point: vendor/runner/cli.js
Runner class: vendor/runner/runner.js
Service tester class: vendor/runner/service-tester.js
Vendor spec: vendor/cran.js
Documentation and tutorial: vendor/README.md

Todos

Document
- Guide to writing tests
Merge conflicting work and dependencies
- Store raster badges in the LRU cache #932
- Improve CTAN error handling and fix non-svg 404 badges #922
Follow up on upstream PRs
- baseUri function to override global setup IcedFrisby/IcedFrisby#26 (required)
- Add before callback to support plugins IcedFrisby/IcedFrisby#28 (required)
- Export Frisby constructor to support plugins IcedFrisby/IcedFrisby#29 (nice to have)
- Add finally() hook for cleanup IcedFrisby/IcedFrisby#45 (nice to have)
Running
- Schedule this to run periodically (vendor tests will run in scheduled daily build of master)
- Create a way to trigger the affected vendors from PRs (nice to have)
Open issues
- Decide how to handle printable description vs short name vs path prefix on the CLI

paulmelnikow · 2017-04-13T16:53:06Z

@niccokunzmann Since you like working on documentation, could I ask you take a look at this?

What would be really awesome is, if you could pick a service, follow the tutorial, and see how you make out!

niccokunzmann · 2017-04-14T06:02:18Z

I think,

the tutorial should be moved to the TUTORIAL file to give it more presence and have the docs all together.
the vendor readme should then link to the tutorial.

I want to create more PRs

niccokunzmann · 2017-04-14T06:49:56Z

vendor/README.md

+In this tutorial, we'll write tests for the Travis badge:
+
+```js
+camp.route(/^\/travis(-ci)?\/([^\/]+\/[^\/]+)(?:\/(.+))?\.(svg|png|gif|jpg|json)$/,


In this example, I would like to discuss what needs testing and what not.
This can be done with the // (1) annotations.

Addressed in 1002045.

niccokunzmann · 2017-04-14T06:56:59Z

vendor/README.md

+  .get('/rust-lang/rust.json')
+  .expectJSONTypes(Joi.object().keys({
+    name: Joi.equal('build'),
+    value: Joi.equal('failing', 'passing', 'unknown')


What does this mean?

Addressed in 555251d.

niccokunzmann · 2017-04-14T07:04:32Z

If you merge this without the travis test being implemented, the commit can be checked out and the tutorial can be continued from this. https://docs.djangoproject.com/en/dev/intro/contributing/#rolling-back-to-a-previous-revision-of-django

niccokunzmann · 2017-04-14T06:57:05Z

vendor/README.md

+
+```js
+t.create('build status on default branch')
+  .get('/rust-lang/rust.json')


Why do we need rust?

I understand! https://github.com/badges/shields/pull/937/files#diff-2bcb895f83ee1e2e5fc386962bf9d6c0R157
This is the url we want to test.

Addressed in 5e9ec62.

niccokunzmann · 2017-04-14T06:57:46Z

vendor/README.md

+
+When defining an IcedFrisby test, typically you would invoke the `toss()`
+method, to register the test. However, this is not necessary when using
+ServiceTester.


Why? Is it implemented?

Addressed in 8b14abe.

niccokunzmann · 2017-04-14T06:58:28Z

vendor/README.md

+Run the test:
+
+```
+npm run test:vendor -- --only=travis


This is a small "travis" above, we used thebig travis. Is this referring to /travis or the name?

Added this as a todo above.

niccokunzmann · 2017-04-14T07:01:44Z

vendor/README.md

+```
+
+
+Getting help


This could be at the beginning. If I needed help during the tutorial, I would not have reached this.

paulmelnikow · 2017-04-17T03:17:55Z

@niccokunzmann Just wanted to let you know I would be sitting down to incorporate your comments. Thank you. They were all super helpful! And, glad you want to create more PRs!

niccokunzmann · 2017-04-17T07:05:22Z

The PRs are here: paulmelnikow/shields#1 paulmelnikow/shields#2 paulmelnikow/shields#3
They go into your pull-request.

paulmelnikow · 2017-04-17T14:13:24Z

Nice work! I added you as a collaborator on my fork. Feel free to push more commits to this branch if you'd like.

paulmelnikow · 2017-04-20T13:52:38Z

Should we change the vendor-badge issue tag to service?

# Conflicts: # doc/TUTORIAL.md

paulmelnikow · 2017-04-28T03:13:41Z

Merged this! Thanks @niccokunzmann and @Daniel15.

Add file size test

Also, include `lib/suggest.js` which is part of the server, and rename `vendor` to `service-tests` to match the renames which occurred during the review of #937.

paulmelnikow added 16 commits April 7, 2017 20:42

Cherry-pick some test helpers

a74d96d

Integrate vendor tests

2d0ae03

Allow running a single test suite

645c233

Minor doc cleanup

dacaa41

More doc

17627cb

Pull icedfrisby-nock from npm

112f30e

Merge branch 'master' into vendor-tests

cf96bf1

Make only() work as intended

60787a3

Merge branch 'master' into vendor-tests

e8d59b8

Add coverage script

fc2565e

Add tutorial on writing vendor tests, first pass

b08bea1

Edits to the readme

f764a25

Readme: Add a complete example

1d86c50

Reorder sections

ef082d2

Merge branch 'master' into vendor-tests

04625c0

Enable test for 404 badge now that #922 is merged

fa13c8e

niccokunzmann reviewed Apr 14, 2017

View reviewed changes

paulmelnikow added 2 commits April 15, 2017 11:45

Unfork icedfrisby

2fc06f5

Bump is-png to check the entire PNG signature

dedce45

niccokunzmann added 2 commits April 17, 2017 10:05

Add a section for tests in the tutorial

1f070a0

comment on boiler plate code

1b297f7

paulmelnikow added the developer-experience Dev tooling, test framework, and CI label Apr 17, 2017

paulmelnikow added 12 commits April 19, 2017 16:48

Use function decls

cd5c70a

Clarify about choosing Rust

0bd15a7

vendor -> service-tests

34d0c5c

Alphabetize requires

e5a37ae

More function decls

a61b931

Add a comment to clarify

6a3521f

Parse args once

c9df451

Don’t pass extra arguments from map()

f7ad7da

uniq -> Set

a55465b

Prevent calling start() more than once

43585c0

vendor -> services in travis.yml

6ad2171

Missed vendor -> service

509e332

paulmelnikow mentioned this pull request Apr 20, 2017

Fix bug in website badge, allow specifying website name (left part of badge) #949

Merged

Daniel15 approved these changes Apr 23, 2017

View reviewed changes

paulmelnikow mentioned this pull request Apr 25, 2017

GitHub (pre-)releases #959

Merged

paulmelnikow added 3 commits April 25, 2017 12:52

Use node-fetch

7f307f2

Clean lint

d9e0196

Merge branch 'master' into vendor-tests

71458a6

# Conflicts: # doc/TUTORIAL.md

paulmelnikow merged commit 5c147b8 into badges:master Apr 28, 2017

webcaetano added a commit to webcaetano/shields that referenced this pull request Apr 29, 2017

badges#745 | badges#937 : create github service-test

ca67136

Add file size test

webcaetano mentioned this pull request Apr 29, 2017

[Github] Populate Service Test #976

Closed

webcaetano added a commit to webcaetano/shields that referenced this pull request Apr 29, 2017

badges#745 | badges#937 : create github service-test

3ed09b1

Add file size test

webcaetano added a commit to webcaetano/shields that referenced this pull request Apr 29, 2017

badges#745 | badges#937 : create github service-test

80e042b

Add file size test

paulmelnikow mentioned this pull request Apr 30, 2017

Github rate limits cause transient service test failures in CI #979

Closed

paulmelnikow deleted the vendor-tests branch June 13, 2017 14:21

paulmelnikow added a commit that referenced this pull request Oct 9, 2017

Exclude tests from coverage (#1136)

40f7147

Also, include `lib/suggest.js` which is part of the server, and rename `vendor` to `service-tests` to match the renames which occurred during the review of #937.

paulmelnikow changed the title ~~Vendor tests~~ Service tests Dec 6, 2017

platan mentioned this pull request Feb 27, 2018

Travis service tests from tutorial #1531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service tests #937

Service tests #937

paulmelnikow commented Apr 8, 2017 •

edited

Loading

paulmelnikow commented Apr 13, 2017

niccokunzmann commented Apr 14, 2017 •

edited

Loading

niccokunzmann Apr 14, 2017

paulmelnikow Apr 17, 2017

niccokunzmann Apr 14, 2017

paulmelnikow Apr 17, 2017

niccokunzmann commented Apr 14, 2017 •

edited

Loading

niccokunzmann Apr 14, 2017

niccokunzmann Apr 14, 2017

paulmelnikow Apr 17, 2017

niccokunzmann Apr 14, 2017

paulmelnikow Apr 17, 2017

niccokunzmann Apr 14, 2017

paulmelnikow Apr 17, 2017

niccokunzmann Apr 14, 2017

paulmelnikow commented Apr 17, 2017

niccokunzmann commented Apr 17, 2017 •

edited

Loading

paulmelnikow commented Apr 17, 2017

paulmelnikow commented Apr 20, 2017 •

edited

Loading

paulmelnikow commented Apr 28, 2017

		```


		Getting help

Service tests #937

Service tests #937

Conversation

paulmelnikow commented Apr 8, 2017 • edited Loading

Goals

Features

Solution

Suggested review order

Todos

paulmelnikow commented Apr 13, 2017

niccokunzmann commented Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niccokunzmann commented Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented Apr 17, 2017

niccokunzmann commented Apr 17, 2017 • edited Loading

paulmelnikow commented Apr 17, 2017

paulmelnikow commented Apr 20, 2017 • edited Loading

paulmelnikow commented Apr 28, 2017

paulmelnikow commented Apr 8, 2017 •

edited

Loading

niccokunzmann commented Apr 14, 2017 •

edited

Loading

niccokunzmann commented Apr 14, 2017 •

edited

Loading

niccokunzmann commented Apr 17, 2017 •

edited

Loading

paulmelnikow commented Apr 20, 2017 •

edited

Loading