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 1431259: add headless tests for cdn #4765

Merged
merged 1 commit into from May 8, 2018

Conversation

Projects
None yet
3 participants
@escattone
Member

escattone commented May 4, 2018

This is the state of the browserless CDN tests after about 3 days. The tests cover every behavior in the CDN except for static/*, and cover a significant portion of the endpoints. There are certainly more tests that could be developed to check specific endpoints for cache variation based on specific headers or cookies or query parameters, but these are a decent start I think.

During the course of writing these tests, I found another bug with the current configuration of the CDN, namely that when editing based on a revision (e.g., https://developer.allizom.org/en-US/docs/Web/HTML$edit/1299417) the publish will be met with a permission-denied error. This is due to the fact that this endpoint bypasses the *$edit behavior, a pass-through behavior which forwards the csrftoken cookie, and falls instead into the default behavior, which does not forward the csrftoken cookie. I filed this bug as bugzilla #1459087.

@jwhitlock

Thanks @escattone, this looks like a good framework for testing these headers. It will be next week before I can dig in.

I think this would be a good opportunity to use a dynamic fixture. If the _kuma_status.json call returns headers like x-cache that show the webserver is behind CloudFront, then expect CloudFront caching headers. Otherwise, expect s-maxage, etc.

return response
@pytest.mark.headless

This comment has been minimized.

@jwhitlock

jwhitlock May 4, 2018

Member

Do you use these mark tags? I haven't incorporated them into my workflow yet.

This comment has been minimized.

@escattone

escattone May 4, 2018

Member

No, I don't use them. I was like a sheep following along with the other headless tests! 🐑 In fact, now that I think of it, I think we should dump them, since I don't think they're really that useful. What do you think?

This comment has been minimized.

@jwhitlock

jwhitlock May 4, 2018

Member

Let's remove them until we use them. I thought nondestructive was used somewhere (Jenkins?), but I think it may be the same a login tests and unused.

@escattone

This comment has been minimized.

Member

escattone commented May 4, 2018

@jwhitlock That's a good idea to use a dynamic fixture. I'll look into that.

@escattone

This comment has been minimized.

Member

escattone commented May 5, 2018

I force pushed an update.

@codecov-io

This comment has been minimized.

codecov-io commented May 7, 2018

Codecov Report

Merging #4765 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4765   +/-   ##
=======================================
  Coverage   95.82%   95.82%           
=======================================
  Files         270      270           
  Lines       24568    24568           
  Branches     1750     1750           
=======================================
  Hits        23542    23542           
  Misses        814      814           
  Partials      212      212

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 4a34764...d7751b8. Read the comment docs.

@jwhitlock

An update to pytest (3.1.0 seems to work) is needed, or remove pytest.param.

I ran pytest tests/headless --reruns=1 --base-url=<url to test>. Retries were needed to deal with staging 504s. This ran with no errors with --base-url=https://developer.mozilla.org and --base-url=https://developer.allizom.org.

With --base-url=http://localhost:8000, I got errors for:

  • /diagrams/workflow/workflow.svg
  • /presentations/microsummaries/index.html
  • /en-US/docs/Web/HTML$revision/1293895
  • /en-US/docs/Web/HTML$compare?locale=en-US&to=1299417&from=1293895
  • /en-US/docs/Learn/CSS/Styling_text/Fundamentals#Color
  • /en-US/docs/Learn/CSS/Styling_text/Fundamentals$toc
  • /files/2767/hut.jpg
  • /@api/deki/files/3613/=hut.jpg

I think some can be fixed by ensuring those pages are in the sample database, but others (legacy files, attachments) won't work in the development environment by default. I think that follow-on work can be done in a new PR, and I can do it.

@pytest.mark.nondestructive
@pytest.mark.parametrize(
'slug, params',
[pytest.param('/en-US/dashboards/topic_lookup', {'topic': 'mathml'},

This comment has been minimized.

@jwhitlock

jwhitlock May 8, 2018

Member

pytest.param is not in pytest 3.0.7. I think it was introduced in pytest 3.1.0.

This comment has been minimized.

@escattone

escattone May 8, 2018

Member

Oops, thanks! I was using pytest (3.5.1) locally!

@pytest.mark.parametrize(
'slug, params',
[pytest.param('/en-US/dashboards/topic_lookup', {'topic': 'mathml'},
marks=pytest.mark.skip), # Gets 504 due to slow response

This comment has been minimized.

@jwhitlock

jwhitlock May 8, 2018

Member

What's your reasoning for adding a test that we are skipping from the start?

This comment has been minimized.

@escattone

escattone May 8, 2018

Member

I wanted to make this an xfail, since most of the time the endpoint is so slow the CDN returns a 504 (the CDN is already configured at the maximum timeout of 60 seconds), but that adds over a minute to the tests, so I thought I'd add it as a skip as a reminder that this endpoint has issues. Perhaps it's best to remove for now, and put this information in a # TODO: ... comment?

This comment has been minimized.

@jwhitlock

jwhitlock May 8, 2018

Member

I could go with skip or comment it out. Skip makes it more likely we'll remember to unskip it later, if we improve performance of topic_lookup. A comment means you don't have to update pytest.

This comment has been minimized.

@escattone

escattone May 8, 2018

Member

I'll go the route of updating pytest, thanks @jwhitlock.

@escattone

This comment has been minimized.

Member

escattone commented May 8, 2018

This is ready for review again @jwhitlock. Sorry, I force-pushed a rebase by habit, but had wanted to add a separate commit for the pytest upgrade to version 3.1.3.

pytest==3.0.7 \
--hash=sha256:66f332ae62593b874a648b10a8cb106bfdacd2c6288ed7dec3713c3a808a6017 \
--hash=sha256:b70696ebd1a5e6b627e7e3ac1365a4bc60aaf3495e843c1e70448966c5224cab
# Code: https://github.com/pytest-dev/pytest

This comment has been minimized.

@jwhitlock

jwhitlock May 8, 2018

Member

😍 added the URLS!

@jwhitlock

Works with pytest 3.1.3. Thanks @escattone!

@jwhitlock jwhitlock merged commit 907c7ec into mozilla:master May 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json No dependency changes
Details

@escattone escattone deleted the escattone:cdn-headless-tests-1431259 branch May 21, 2018

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