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

5186 set up JavaScript test infrastructure #5207

Merged
merged 7 commits into from
Jul 29, 2018

Conversation

TimKam
Copy link
Member

@TimKam TimKam commented Jul 22, 2018

Subject: Set up JavaScript test infrastructure

Feature or Bugfix

  • Maintenance

Purpose

It would be good to have tests for Sphinx's JavaScript code. The goal of this PR is to set up the necessary infrastructure for this. Later, we can gradually increase test coverage.

Detail

  • Refactor JavaScript code to minimize code in template files
  • Configure karma.js and write first test
  • Configure travis-CI

Notes

  • It looks strange at first glance, but package-lock.json should in fact be committed, see: npm documentation

Relates

@TimKam TimKam force-pushed the 5186-js-test-infrastructure branch from 5bfee3f to 2fa3826 Compare July 22, 2018 17:20
@codecov
Copy link

codecov bot commented Jul 22, 2018

Codecov Report

Merging #5207 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5207      +/-   ##
==========================================
+ Coverage   82.32%   82.34%   +0.01%     
==========================================
  Files         297      297              
  Lines       39333    39446     +113     
  Branches     6051     6068      +17     
==========================================
+ Hits        32380    32480     +100     
- Misses       5618     5635      +17     
+ Partials     1335     1331       -4
Impacted Files Coverage Δ
sphinx/ext/mathbase.py 72.88% <0%> (-3.39%) ⬇️
tests/test_autodoc.py 94.5% <0%> (-1.79%) ⬇️
sphinx/writers/html.py 90.54% <0%> (-0.92%) ⬇️
sphinx/writers/html5.py 83.67% <0%> (-0.75%) ⬇️
tests/test_build_html.py 97.12% <0%> (-0.45%) ⬇️
tests/test_application.py 98.61% <0%> (-0.02%) ⬇️
tests/test_environment_toctree.py 100% <0%> (ø) ⬆️
tests/test_ext_doctest.py 95.45% <0%> (ø) ⬆️
tests/test_ext_apidoc.py 94.49% <0%> (ø) ⬆️
sphinx/registry.py 71.23% <0%> (+0.1%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b4a4a9...afe46ef. Read the comment docs.

@TimKam TimKam force-pushed the 5186-js-test-infrastructure branch from 0d0bdb7 to 5ed3f0a Compare July 22, 2018 18:51
@TimKam TimKam changed the title WIP: 5186 set up JavaScript test infrastructure 5186 set up JavaScript test infrastructure Jul 22, 2018
@TimKam
Copy link
Member Author

TimKam commented Jul 22, 2018

@tk0miya Can you give feedback? If this PR goes into the right direction, I can continue adding tests.

@tk0miya
Copy link
Member

tk0miya commented Jul 23, 2018

It seems there are no test runners for node_js in scripts: section. Would you like to add them in another PR? If intended, I'm okay to this. Please go ahead.

@TimKam
Copy link
Member Author

TimKam commented Jul 23, 2018

The default Node.js build script is npm test, see here in the Travis-CI docs. That's why I think it's not necessary. The test runs without it. Or am I missing something?

@tk0miya
Copy link
Member

tk0miya commented Jul 24, 2018

Please read the log for node_js job.
https://travis-ci.org/sphinx-doc/sphinx/jobs/406888385

We've already overrided it by travis.yml. So default setting is not used. I think you need to call manually like this:

  - if [ $RUN_TOX = false ]; then npm test; fi

I know $RUN_TOX is not good naming for such case.

@TimKam TimKam force-pushed the 5186-js-test-infrastructure branch from 9e8db5f to fcd0b78 Compare July 24, 2018 19:17
@TimKam
Copy link
Member Author

TimKam commented Jul 24, 2018

Sorry for missing this. Now it works. If you think it's okay to merge, we could do so and I'll add new PRs with more tests every now and then.

 and fix test description of `urldecode()`
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM! Let's merge this :-)

@TimKam TimKam merged commit 33d09e1 into sphinx-doc:master Jul 29, 2018
"inflight": "1.0.6",
"inherits": "2.0.3",
"minimatch": "3.0.4",

Copy link
Member

Choose a reason for hiding this comment

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

I recieved security alert from github.

We found a potential security vulnerability in a repository for which you have been granted security alert access.

Known moderate severity security vulnerability detected in hoek < 4.2.1defined in package-lock.json.
package-lock.json update suggested: hoek ~> 4.2.1.

@TimKam Could you check this please? I don't know about this library so much. But it seems too old.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a problem with dependencies of karma.js, the test runner, see: karma-runner/karma#2994. It is not possible to update the dependency in the test setup. However, I am confident the vulnerabilities cannot be exploited in our usage scenario, as the libraries are not used in a security-critical context. I suggest it is okay to wait for Karma to update the dependencies and to up our Karma version subsequently.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for explanation. I understand. And I agree with you.
Okay, let's keep as is :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants