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

Rewrite Web Connectivity in Go (2/n): QA #810

Closed
bassosimone opened this issue Jul 17, 2020 · 2 comments
Closed

Rewrite Web Connectivity in Go (2/n): QA #810

bassosimone opened this issue Jul 17, 2020 · 2 comments
Assignees
Labels
effort/L Large effort priority/high High priority

Comments

@bassosimone
Copy link
Contributor

We have the code since #776. Now we wanna do QA.

@bassosimone bassosimone added this to the Sprint 18 - Colomber milestone Jul 17, 2020
@bassosimone bassosimone self-assigned this Jul 17, 2020
@bassosimone bassosimone added effort/L Large effort priority/high High priority labels Jul 19, 2020
bassosimone added a commit that referenced this issue Aug 12, 2020
Previous changes made it much more difficult to run on Linux. Yet, to
do QA of Web Connectivity it's more practical to perform it on Linux using
also OONI Probe v2.x and Measurement Kit.

For this reason, this diff implements changes that should allow us to run
both on Linux and when using Docker.

While it's possible to create a Docker container where we deploy compiled
versions of MK and OONI Probe v2.x, it's more practical to use a Linux system
where they are already installed, like my Linux desktop.

Part of #810
bassosimone added a commit that referenced this issue Aug 12, 2020
1. we cannot write on QA/HOME when running on Linux so just
use `/tmp` as the HOME directory (slower but works)

2. the `/tmp` directory may be on another device than `/home`, so
we cannot move, then just perform a copy

Part of #810
bassosimone added a commit that referenced this issue Aug 12, 2020
* QA: make sure we can run from Linux

Previous changes made it much more difficult to run on Linux. Yet, to
do QA of Web Connectivity it's more practical to perform it on Linux using
also OONI Probe v2.x and Measurement Kit.

For this reason, this diff implements changes that should allow us to run
both on Linux and when using Docker.

While it's possible to create a Docker container where we deploy compiled
versions of MK and OONI Probe v2.x, it's more practical to use a Linux system
where they are already installed, like my Linux desktop.

Part of #810

* QA: fix running on Linux

1. we cannot write on QA/HOME when running on Linux so just
use `/tmp` as the HOME directory (slower but works)

2. the `/tmp` directory may be on another device than `/home`, so
we cannot move, then just perform a copy

Part of #810
bassosimone added a commit that referenced this issue Aug 12, 2020
bassosimone added a commit that referenced this issue Aug 12, 2020
This starts adding a single check, to check whether the framework works for Web Connectivity. More checks later.

Part of #810
bassosimone added a commit that referenced this issue Aug 12, 2020
This will be very helpful for #810.

It will allow us, in fact, to compare the result obtained by MK to the one
obtained by miniooni in specific censorship conditions.
bassosimone added a commit that referenced this issue Aug 12, 2020
This will be very helpful for #810.

It will allow us, in fact, to compare the result obtained by MK to the one
obtained by miniooni in specific censorship conditions.
bassosimone added a commit that referenced this issue Aug 13, 2020
Spotted while comparing with Measurement Kit.

Part of #810.
bassosimone added a commit that referenced this issue Aug 13, 2020
According to the implementations, the semantics is that blocking is false
when accessible is true, a nullable string otherwise.

The diff documents in detail how and why this was introduced.

It would probably have been better to not make this field semantics so
complicate, but now it's too late. Maybe in the future we can have some
extra field with a more clear and simple semantics?

Part of #810
bassosimone added a commit that referenced this issue Aug 13, 2020
* fix(webconnectivity): header_match => headers_match

Spotted while comparing with Measurement Kit.

Part of #810.

* webconnectivity: fix the semantics of blocking

According to the implementations, the semantics is that blocking is false
when accessible is true, a nullable string otherwise.

The diff documents in detail how and why this was introduced.

It would probably have been better to not make this field semantics so
complicate, but now it's too late. Maybe in the future we can have some
extra field with a more clear and simple semantics?

Part of #810

* webconnectivity/summary.go: better docstring

* webconnectivity: fix summary_test.go
bassosimone added a commit that referenced this issue Aug 13, 2020
Verified locally that we behave like MK.

Part of #810.
bassosimone added a commit that referenced this issue Aug 13, 2020
Verified locally that we behave like MK.

Part of #810.
bassosimone added a commit that referenced this issue Aug 13, 2020
We already know that probe-engine does better than MK when there is a control
failure and the website is using HTTPS and we can access it.

MK would flag such case as failed measurement, where instead probe-engine
would flag such case as success.

What we can instead test here is what happens if the same happens but we're
accessing a website using HTTP rather than HTTPS.

This QA test allowed us to discover the following issues:

1. MK always sets the body proportion to zero, even when performing such a
comparison is not actually possible, so do the same also in probe-engine even
though probably using `null` would be a more consistent result

2. MK sets the DNSConsistency field only when the control succeded and we
should do the same rather than flagging the DNS as consistent

This diff includes both the diff adding the new QA check and the required fixes
in probe-engine required to address the two above issues.

Part of #810
bassosimone added a commit that referenced this issue Aug 13, 2020
We already know that probe-engine does better than MK when there is a control
failure and the website is using HTTPS and we can access it.

MK would flag such case as failed measurement, where instead probe-engine
would flag such case as success.

What we can instead test here is what happens if the same happens but we're
accessing a website using HTTP rather than HTTPS.

This QA test allowed us to discover the following issues:

1. MK always sets the body proportion to zero, even when performing such a
comparison is not actually possible, so do the same also in probe-engine even
though probably using `null` would be a more consistent result

2. MK sets the DNSConsistency field only when the control succeded and we
should do the same rather than flagging the DNS as consistent

This diff includes both the diff adding the new QA check and the required fixes
in probe-engine required to address the two above issues.

Part of #810
bassosimone added a commit that referenced this issue Aug 13, 2020
We already know that probe-engine does better than MK when there is a control
failure and the website is using HTTPS and we can access it.

MK would flag such case as failed measurement, where instead probe-engine
would flag such case as success.

What we can instead test here is what happens if the same happens but we're
accessing a website using HTTP rather than HTTPS.

This QA test allowed us to discover the following issues:

1. MK always sets the body proportion to zero, even when performing such a
comparison is not actually possible, so do the same also in probe-engine even
though probably using `null` would be a more consistent result

2. MK sets the DNSConsistency field only when the control succeded and we
should do the same rather than flagging the DNS as consistent

This diff includes both the diff adding the new QA check and the required fixes
in probe-engine required to address the two above issues.

Part of #810
bassosimone added a commit that referenced this issue Aug 14, 2020
This is a rather common blocking case. Let us make sure we
measure this case with OPE like we do with MK.

I needed to remove the checks pertaining to the in depth
structure of the JSON, because fields are missing in JSON
files generated by MK. We know these bugs.

Part of #810
bassosimone added a commit that referenced this issue Aug 14, 2020
This is a rather common blocking case. Let us make sure we
measure this case with OPE like we do with MK.

I needed to remove the checks pertaining to the in depth
structure of the JSON, because fields are missing in JSON
files generated by MK. We know these bugs.

Part of #810
@bassosimone
Copy link
Contributor Author

Also #844 was part of this issue.

bassosimone added a commit that referenced this issue Aug 14, 2020
Make sure we do the same MK would do in this case.

Part of #810.
bassosimone added a commit that referenced this issue Aug 14, 2020
Make sure we do the same MK would do in this case.

Part of #810.
bassosimone added a commit that referenced this issue Aug 14, 2020
Part of #810

Objective is making sure we don't do different from MK as much
as possible as far as we can predict censorship cases.
bassosimone added a commit that referenced this issue Aug 14, 2020
Part of #810

Objective is making sure we don't do different from MK as much
as possible as far as we can predict censorship cases.
bassosimone added a commit that referenced this issue Aug 14, 2020
Part of #810

Strictly speaking we don't need a redirect chain to perform
this measurement but we have been hiding the initial request
from TCP/IP errors, so let's keep using that pattern.
bassosimone added a commit that referenced this issue Aug 14, 2020
Part of #810

Strictly speaking we don't need a redirect chain to perform
this measurement but we have been hiding the initial request
from TCP/IP errors, so let's keep using that pattern.
bassosimone added a commit that referenced this issue Aug 14, 2020
Again, likely doing the redirect chain is not necessary. But it makes
things more interesting, because we are testing stuff that happens after
the initial DNS, TCP, etc. checks performed by Web Connectivity.

Part of #810
bassosimone added a commit that referenced this issue Aug 14, 2020
Again, likely doing the redirect chain is not necessary. But it makes
things more interesting, because we are testing stuff that happens after
the initial DNS, TCP, etc. checks performed by Web Connectivity.

Part of #810
@bassosimone
Copy link
Contributor Author

I've been doing a bunch of work in Sprint 19. The work is not complete, though, so I created a new issue to keep track of the additional QA efforts required before declaring the reimplementation successful: #852.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/L Large effort priority/high High priority
Projects
None yet
Development

No branches or pull requests

1 participant