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

bug 924958: first-pass at test framework for macros #204

Merged
merged 2 commits into from Jun 19, 2017

Conversation

Projects
None yet
3 participants
@escattone
Member

escattone commented Jun 13, 2017

This PR defines a set of functions that constitute a first pass at a test framework for macros, as well as full test-suites for five frequently-used macros that show how to use the framework in practice. The goal was to be able to handle the complexity of testing sophisticated macros that often require mocking external services, while at the same time keeping things as simple as possible. I don't consider this a final product, but just the start of a conversation towards one.

  • tests/macros/utils.js defines a set of functions
    that comprise a mocha-based test framework for
    simplifying the testing of Kumascript macros
  • fixes the EJSTemplate's execute method so
    that it no longer re-calls its callback if the
    callback itself throws an error
  • adds a make test-macros command
  • adds an explicit timeout, which defaults to 10s,
    to the test commands (fixes an issue where some
    some tests would occasionally exceed the default
    mocha timeout of 2s)
  • exercises the new test framework with tests (in
    the tests/macros/ directory) for five simple
    macros that are frequently used
  • adds "chai-as-promised" npm package to "package.json"
  • add make test-macros to the "Jenkinsfile" and
    ".travis.yml" files.

@escattone escattone requested review from jwhitlock, Elchi3 and wbamberg Jun 13, 2017

@jwhitlock

👍 I like how the tests look, and I can imagine our mid-to-highly experienced contributors updating and adding tests. I'd be happy to merge, so consider my comments nitpicks.

On the Django side, I like to avoid making real network requests, for test speed and to avoid a source of random failures. I'd be happy to leave that for now as a suggestion for test writers, rather than something enforced by the framework. But, I'll be watching for when we need to start enforcing it 👀 .

@@ -11,6 +11,7 @@ before_script:
script:
- make test
- make lint-macros
- make test-macros

This comment has been minimized.

@jwhitlock

jwhitlock Jun 13, 2017

Member

👍 Tests pass in TravisCI, and the test output looks good.

@jwhitlock

jwhitlock Jun 13, 2017

Member

👍 Tests pass in TravisCI, and the test output looks good.

return assert.eventually.equal(
macro.call('1.9.2'),
`<span class="inlineIndicator deprecated deprecatedInline" title="(Firefox 3.6 / Thunderbird 3.1 / Fennec 1.0)">Deprecated since Gecko 1.9.2</span>`
);

This comment has been minimized.

@jwhitlock

jwhitlock Jun 13, 2017

Member

I think this is a clear way to test these short, inline macros. 👍

@jwhitlock

jwhitlock Jun 13, 2017

Member

I think this is a clear way to test these short, inline macros. 👍

return assert.eventually.equal(
macro.call(),
`<div class="overheadIndicator draft">\n <p><strong>Borrador</strong><br/>\n Esta página no está completa.</p>\n \n</div>`
);

This comment has been minimized.

@jwhitlock

jwhitlock Jun 13, 2017

Member

I like this method for setting the environment locale. I'm less comfortable with exact string matching for HTML, but in the context of macros, it may be OK. I'm not looking forward to sidebar testing 😟, but I think we can load the expected content from a file, to keep the test code short.

@jwhitlock

jwhitlock Jun 13, 2017

Member

I like this method for setting the environment locale. I'm less comfortable with exact string matching for HTML, but in the context of macros, it may be OK. I'm not looking forward to sidebar testing 😟, but I think we can load the expected content from a file, to keep the test code short.

This comment has been minimized.

@escattone

escattone Jun 13, 2017

Member

I totally agree with your concerns. We can easily switch to a slightly different style (where we add a handler to the promise returned from macro.call that does various checks on the actual result) if we'd rather check for specific elements in the HTML rather than do a simple exact string match.

@escattone

escattone Jun 13, 2017

Member

I totally agree with your concerns. We can easily switch to a slightly different style (where we add a handler to the promise returned from macro.call that does various checks on the actual result) if we'd rather check for specific elements in the HTML rather than do a simple exact string match.

Show outdated Hide outdated tests/macros/test-httpheader.js
Show outdated Hide outdated tests/macros/test-specname.js
Show outdated Hide outdated tests/macros/utils.js
@@ -12,6 +12,7 @@ DEIS_PROFILE ?= usw
DEIS_APP ?= kumascript-dev
PRIVATE_IMAGE ?= ${PRIVATE_REGISTRY}/${DEIS_APP}\:${VERSION}
TEST_RUN_ARGS ?=
TEST_RUN_TIMEOUT ?= 10000

This comment has been minimized.

@jwhitlock

jwhitlock Jun 13, 2017

Member

This worries me a little. Are there tests that are making real network calls to MDN or GitHub? It makes me think we should auto-stub some of the API calls to fail if they are called "for real" rather than with stub data. We can re-evaluate if it becomes a problem.

@jwhitlock

jwhitlock Jun 13, 2017

Member

This worries me a little. Are there tests that are making real network calls to MDN or GitHub? It makes me think we should auto-stub some of the API calls to fail if they are called "for real" rather than with stub data. We can re-evaluate if it becomes a problem.

This comment has been minimized.

@escattone

escattone Jun 13, 2017

Member

I changed this timeout setting specifically for the server code tests (make test) rather than the macro tests (make test-macros). None of the macro tests currently make real network calls (e.g., test-httpheader.js stubs the wiki.getPage calls). I had debated making separate timeouts for each, but decided we could do that later if we thought it best.

@escattone

escattone Jun 13, 2017

Member

I changed this timeout setting specifically for the server code tests (make test) rather than the macro tests (make test-macros). None of the macro tests currently make real network calls (e.g., test-httpheader.js stubs the wiki.getPage calls). I had debated making separate timeouts for each, but decided we could do that later if we thought it best.

This comment has been minimized.

@jwhitlock

jwhitlock Jun 13, 2017

Member

OK, that makes sense. Thanks for the explanation!

@jwhitlock

jwhitlock Jun 13, 2017

Member

OK, that makes sense. Thanks for the explanation!

@jwhitlock

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock Jun 13, 2017

Member

Thanks @escattone, looks good! Let's see what @Elchi3 and @wbamberg think...

Member

jwhitlock commented Jun 13, 2017

Thanks @escattone, looks good! Let's see what @Elchi3 and @wbamberg think...

escattone added some commits Jun 7, 2017

bug 924958: first-pass at test framework for macros
* tests/macros/utils.js defines a set of functions
  that comprise a mocha-based test framework for
  simplifying the testing of Kumascript macros
* fixes the EJSTemplate's "execute" method so
  that it no longer re-calls its callback if the
  callback itself throws an error
* adds a "test-macros" command to the Makefile
* adds an explicit timeout, which defaults to 10s,
  to the test commands (fixes an issue where some
  some tests would occassionly exceed the default
  mocha timeout of 2s)
* exercises the new test framework with tests (in
  the tests/macros/ directory) for five simple
  macros that are frequently used
* adds "chai-as-promised" npm package to "package.json"
* add "make test-macros" to the "Jenkinsfile" and
  ".travis.yml" files.
address review suggestions
* use multi-line comments
* use real examples (where possible)
* move mock fixture setup into
  its specific test case
@Elchi3

This comment has been minimized.

Show comment
Hide comment
@Elchi3

Elchi3 Jun 15, 2017

Member

I think this is an excellent start towards testing macros. From the perspective of someone who wants to write more tests and new macros, I would appreciate documentation about how to do that exactly. It's great to see that we are using a popular library like mocha, but I'm actually not very familiar with it. So pointers to the mocha docs in our docs would be nice, too. From reading the code in this PR, I think I know what's going on, though I guess questions will come up, when I'm writing my first own test.

Maybe a we need a whole new .md document on how to contribute macros. In there a section on how to write tests and what our code style best practices (in the newer ES6 world) are. A workshop about this at a co-incidental work-week would be nice, too.

Great work. I'm more hopeful about this codebase becoming sane one day.

Member

Elchi3 commented Jun 15, 2017

I think this is an excellent start towards testing macros. From the perspective of someone who wants to write more tests and new macros, I would appreciate documentation about how to do that exactly. It's great to see that we are using a popular library like mocha, but I'm actually not very familiar with it. So pointers to the mocha docs in our docs would be nice, too. From reading the code in this PR, I think I know what's going on, though I guess questions will come up, when I'm writing my first own test.

Maybe a we need a whole new .md document on how to contribute macros. In there a section on how to write tests and what our code style best practices (in the newer ES6 world) are. A workshop about this at a co-incidental work-week would be nice, too.

Great work. I'm more hopeful about this codebase becoming sane one day.

@Elchi3

Elchi3 approved these changes Jun 15, 2017

@escattone

This comment has been minimized.

Show comment
Hide comment
@escattone

escattone Jun 15, 2017

Member

Thanks for your review @Elchi3! Yes, I totally agree with you on documentation. I was thinking we could work on that in another PR if this test framework seemed acceptable, but perhaps I should add some initial documentation to this PR?

Member

escattone commented Jun 15, 2017

Thanks for your review @Elchi3! Yes, I totally agree with you on documentation. I was thinking we could work on that in another PR if this test framework seemed acceptable, but perhaps I should add some initial documentation to this PR?

@Elchi3

This comment has been minimized.

Show comment
Hide comment
@Elchi3

Elchi3 Jun 19, 2017

Member

We can merge this and add docs in another PR.

We should require tests for new macros, but we don't write new macros very often, so I think it's fine to add the docs in a follow-up.

Member

Elchi3 commented Jun 19, 2017

We can merge this and add docs in another PR.

We should require tests for new macros, but we don't write new macros very often, so I think it's fine to add the docs in a follow-up.

@jwhitlock

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock Jun 19, 2017

Member

Sounds like a plan. Should docs be with the rest of the developer docs at https://kuma.readthedocs.io, or should we continue growing the readme?

Member

jwhitlock commented Jun 19, 2017

Sounds like a plan. Should docs be with the rest of the developer docs at https://kuma.readthedocs.io, or should we continue growing the readme?

@jwhitlock jwhitlock merged commit 558e54d into mdn:master Jun 19, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@Elchi3

Elchi3 Jun 20, 2017

Member

Should docs be with the rest of the developer docs at https://kuma.readthedocs.io, or should we continue growing the readme?

I'd like to keep the docs in this repo. It allows making docs changes in the same PR as we change the code. Maybe it's time for a docs/ folder here, though. We could add a few files:

  • Architecture.md (diagram, how KS works)
  • Development.md (docker, setup, etc)
  • UpdatingMacros.md (the most common workflow explained)
  • TestingMacros.md (test infra, mocha, how to write tests, fixtures etc. explained)
  • Contributing.md (points to development.md or UpdatingMacros.md or other docs depending on what you want to contribute).
Member

Elchi3 commented Jun 20, 2017

Should docs be with the rest of the developer docs at https://kuma.readthedocs.io, or should we continue growing the readme?

I'd like to keep the docs in this repo. It allows making docs changes in the same PR as we change the code. Maybe it's time for a docs/ folder here, though. We could add a few files:

  • Architecture.md (diagram, how KS works)
  • Development.md (docker, setup, etc)
  • UpdatingMacros.md (the most common workflow explained)
  • TestingMacros.md (test infra, mocha, how to write tests, fixtures etc. explained)
  • Contributing.md (points to development.md or UpdatingMacros.md or other docs depending on what you want to contribute).

jwhitlock added a commit to jwhitlock/kuma that referenced this pull request Jun 20, 2017

Update Kumascript (PRs 165, 204, 205, 208, 5 more)
* mdn/kumascript#165 - csssyntax: Only link to named colors
* mdn/kumascript#204 - Macro testing framework
* mdn/kumascript#205 - CSSRef, CSS_Ref: Prep for title change
* mdn/kumascript#208 - CSS_Ref, csssyntax: Prep for syntax change
* mdn/kumascript#210 - GroupData, InterfaceData, etc: add Long Tasks API
* mdn/kumascript#211 - spec2, etc: Add * WEBGL_compressed_texture_s2tc_srgb
* mdn/kumascript#212 - Firefox_for_developers: Pass max version
* mdn/kumascript#214 - Add liveness, readiness endpoints
* mdn/kumascript#216 - SpecName, spec2: Update performance specs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment