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

add http-observatory tests #518

Closed
wants to merge 2 commits into from
Closed

Conversation

@jarondl
Copy link

jarondl commented Oct 20, 2016

Add a locally (docker) deployed http-observatory test.
It requires a minimum score of 65, and also prints the test results to
stdout.
Helps with #473.
Will fail on travis for two reasons:

  1. #505 - buildbot does not run automatically on travis right now
  2. #511 - the security headers were not merged yet, so our score is 0

The code is not so elegant, running external code from within Python, but it works.


This change is Reviewable

Add a locally (docker) deployed http-observatory test.
It requires a minimum score of 65, and also prints the test results to
stdout.
@highfive
Copy link

highfive commented Oct 20, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @aneeshusa (or someone else) soon.

@aneeshusa
Copy link
Member

aneeshusa commented Oct 30, 2016

I'm OK with relying on the public Mozilla API instance instead of running our own in CI. I would much prefer to avoid Docker, and it is optional for the cli client, so please rework this to avoid the docker and docker-compose dependencies.

I believe that Travis uses a single mega-image for Trusty builds (which we are using), which already includes Node (for the cli client), although I might be wrong. No need to add Node to the Vagrant setup for now, since running the test suite in Vagrant already requires some extra undocumented steps.

@jarondl
Copy link
Author

jarondl commented Oct 30, 2016

Hi @aneeshusa,
The trouble with using the public API, is that it only checks public urls. That is the only reason I went down this much more complicated path of implementing our own CI.

So if we want to avoid docker and use the public api we can either:

  1. Run the tests targeting the already deployed server. Quite weird because there is no connection between changing the code and the test results.
  2. Run a publicly accessible staging server. Even more complicated than the docker solution, unsafe (the server runs publicly even if the tests failed).

Have I missed anything?

BTW, docker is also available on travis.

@aneeshusa
Copy link
Member

aneeshusa commented Oct 30, 2016

@jarondl That makes sense, I didn't think about that at 3am when making my last comment :)

I appreciate all the work you've done on this PR, but I'm still unconvinced that we need Docker running Postgres, Redis and Celery all to scan one website in CI; we don't need to save results or handle backpressure, etc.
Since this test isn't critical to saltfs functionality (aka builds getting run and merged), and we can always use the website to check our deployed site, I would rather not have this test at all than include all of that complexity (especially considering the fact that we can add simple checks for e.g. headers ourselves as in your other PR).

I also see you've asked them to start building official Docker images, but in the meantime I am reluctant to use a community image (even one you built!). I would want to either a) wait for an official image or b) have a script in saltfs to build + store one; more importantly I would want to pin the image via digest in all cases to avoid unnecessary surprises, which is then another thing to update, etc.

I'm not sure about the http-observatory internals, but assuming it's well architected I'm hoping there is a (even if unofficial) Python API we can use to grade a single site directly, without hooking into Postgres etc. If you are willing to figure out if/how that can be done, I would be much more amenable to integrating the observatory into our CI that way. (I can also likely poke around a bit.)

@jarondl
Copy link
Author

jarondl commented Oct 31, 2016

Hi @aneeshusa, with some tweaks and fixes ( mozilla/http-observatory#156 ) we could run a Python only scanner in about three lines of code. (Although with an unofficial API - maybe I could push something upstream..)
However, the observatory does require many Python packages we don't have. Should we put it in a separate virtualenv?

@aneeshusa
Copy link
Member

aneeshusa commented Oct 31, 2016

@jarondl Thanks for going above and beyond, pushing improvements to the observatory upstream :) Running a Python only scanner, even if via an unofficial API, would be great.

I'm OK with however many Python dependencies are needed, since these only need to be installed during testing; just drop them into the requirements.txt file at the top of the repo. Travis will install those automatically for our builds marked as Python builds, which includes servo-master1.
Just make sure to pin versions to prevent surprises, especially since we're using an unofficial API.

@aneeshusa
Copy link
Member

aneeshusa commented Oct 31, 2016

One note about integration: the tests already run under a Python3 interpreter, so you should be able to import the observatory modules and call functions directly from the test code, without needing a separate script/virtualenv and forking a separate Python process to run it.

@aneeshusa
Copy link
Member

aneeshusa commented Oct 31, 2016

I filed mozilla/http-observatory#157 about an officially supported Python API, let's see what they say.

@april
Copy link

april commented Nov 4, 2016

The unofficial local API is now official. There is both a function you can call as well as a simple CLI that outputs JSON similar to what the API would output. Hopefully that helps!

@tigercosmos
Copy link

tigercosmos commented Jan 7, 2018

What's the current status?

@aneeshusa
Copy link
Member

aneeshusa commented Jan 7, 2018

@tigercosmos now that there is an official local API for the observatory, this work should be redone to use the official API.
I'm going to close this PR as it's been inactive for quite a while - redoing this is open for anyone to take a stab at!

@aneeshusa aneeshusa closed this Jan 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.