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

Fix display of KumaScript errors for document/preview endpoints #4280

Merged
merged 2 commits into from Jun 21, 2017

Conversation

Projects
None yet
3 participants
@escattone
Member

escattone commented Jun 21, 2017

See bugzilla 1324092.

The fix was easy for this PR, but refactoring the tests proved painful. I added a new wiki-level test fixture ks_toolbox. I placed it at the wiki level assuming it will be used by more than one file that we create as we refactor more of the tests within kuma/wiki/tests/test_views.py. It's currently used by both test_views.py (imported, since the machinery for using fixtures within unittest.TestCase based classes is worse) and test_views_document.py .

@escattone escattone requested a review from jwhitlock Jun 21, 2017

@escattone

This comment has been minimized.

Show comment
Hide comment
@escattone

escattone Jun 21, 2017

Member

The Travis build has failed twice, but I can't reproduce the problem(s) locally. I restarted the build one more time, and I'll look into this tomorrow.

Member

escattone commented Jun 21, 2017

The Travis build has failed twice, but I can't reproduce the problem(s) locally. I restarted the build one more time, and I'll look into this tomorrow.

@jwhitlock jwhitlock self-assigned this Jun 21, 2017

bug 1324092: fix links when document has macro errors
* fix links when displaying/previewing documents
  with kumascript macro errors
* improve/refactor/fix affected tests
@escattone

This comment has been minimized.

Show comment
Hide comment
@escattone

escattone Jun 21, 2017

Member

OK, the test that was failing kuma/wiki/tests/test_views.py::DocumentEditingTests::test_translate_rebuilds_source_json was making multiple rendering calls to the KumaScript service, which in the py27 test env were getting connection errors (which were caught and turned into KumaScript errors) and in the Docker test env were completing without error. So the py27 test env ended up with KumaScript errors which in turn called my new code which calls KumaScript to get the macros (which raised an exception which was not caught). None of the test code relies on the rendered output, so the fix was simple: stop overriding constance.config.KUMASCRIPT_TIMEOUT with a non-zero value so that render requests do not call the KumaScript service. Thanks to @jwhitlock for help with this.

Member

escattone commented Jun 21, 2017

OK, the test that was failing kuma/wiki/tests/test_views.py::DocumentEditingTests::test_translate_rebuilds_source_json was making multiple rendering calls to the KumaScript service, which in the py27 test env were getting connection errors (which were caught and turned into KumaScript errors) and in the Docker test env were completing without error. So the py27 test env ended up with KumaScript errors which in turn called my new code which calls KumaScript to get the macros (which raised an exception which was not caught). None of the test code relies on the rendered output, so the fix was simple: stop overriding constance.config.KUMASCRIPT_TIMEOUT with a non-zero value so that render requests do not call the KumaScript service. Thanks to @jwhitlock for help with this.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 21, 2017

Codecov Report

Merging #4280 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4280      +/-   ##
==========================================
- Coverage   87.87%   87.83%   -0.05%     
==========================================
  Files         162      162              
  Lines       10038    10042       +4     
  Branches     1369     1371       +2     
==========================================
- Hits         8821     8820       -1     
- Misses        990      993       +3     
- Partials      227      229       +2
Impacted Files Coverage Δ
kuma/wiki/views/document.py 90.28% <ø> (-0.24%) ⬇️
kuma/wiki/views/revision.py 92.47% <ø> (ø) ⬆️
kuma/wiki/kumascript.py 83.92% <100%> (-0.83%) ⬇️
kuma/wiki/models.py 89.05% <0%> (-0.23%) ⬇️

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 5890a9f...3452409. Read the comment docs.

codecov-io commented Jun 21, 2017

Codecov Report

Merging #4280 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4280      +/-   ##
==========================================
- Coverage   87.87%   87.83%   -0.05%     
==========================================
  Files         162      162              
  Lines       10038    10042       +4     
  Branches     1369     1371       +2     
==========================================
- Hits         8821     8820       -1     
- Misses        990      993       +3     
- Partials      227      229       +2
Impacted Files Coverage Δ
kuma/wiki/views/document.py 90.28% <ø> (-0.24%) ⬇️
kuma/wiki/views/revision.py 92.47% <ø> (ø) ⬆️
kuma/wiki/kumascript.py 83.92% <100%> (-0.83%) ⬇️
kuma/wiki/models.py 89.05% <0%> (-0.23%) ⬇️

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 5890a9f...3452409. Read the comment docs.

@jwhitlock

This worked well for me. I'm excited to get this error message updated!

I modified some macros so they would fail, and I got the expected links to GitHub. I tried a lowercase macro name, and uppercase one, and 原語併記.ejs

I added {{fail}}, and got the "create macro" variant. GitHub followed the link and then redrew the page, so the section moved down the screen a bit. Not a slam dunk, but I hope someday we'll have RTFD-hosted docs with a better link destination.

One optional nit, and a further change to the translation please.

}
}
return KumaScriptToolbox(errors, errors_as_headers, macros_response)

This comment has been minimized.

@jwhitlock

jwhitlock Jun 21, 2017

Member

Oh, and it took me a few readings to figure out WTF was going on, but good refactor to a py.test fixture 😄

@jwhitlock

jwhitlock Jun 21, 2017

Member

Oh, and it took me a few readings to figure out WTF was going on, but good refactor to a py.test fixture 😄

@escattone

This comment has been minimized.

Show comment
Hide comment
@escattone

escattone Jun 21, 2017

Member

Thanks for the super fast review @jwhitlock! I've addressed your change requests, and it should be ready for you final review.

Member

escattone commented Jun 21, 2017

Thanks for the super fast review @jwhitlock! I've addressed your change requests, and it should be ready for you final review.

@jwhitlock jwhitlock merged commit c1d36a8 into mozilla:master Jun 21, 2017

1 check passed

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

@escattone escattone deleted the escattone:fix-ks-macro-errors-display-1324092 branch Jul 11, 2017

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