943800 - Annotate PSL entries used in test suite, to reduce likelihood of test breakage

Status: RESOLVED WONTFIX

(Core Graveyard :: Networking: Domain Lists, defect)

Description

[Gervase Markham [:gerv]]

We broke the build in bug 880625, by breaking the PSL tests. This is not the first time this has happened. Most PSL updates are straightforward, but the file has a small number of hidden landmines in - TLDs which are used in the test suite. When the entries for those TLDs change, the tests can often break. In this case, it was .cy.

The file netwerk/test/unit/data/test_psl.txt does PSL tests using a number of different TLDs. We should reduce the number of TLDs used in that file to a minimum while still testing all the things we know we want to test, and then we should add a comment above each remaining TLD in the PSL file saying: "Note: this TLD is used in tests. If you change this entry, check the tests still pass."

Gerv

Comment 1

[Simone Carletti]

I believe we should use a different PSL file, a test seed, not the real file. In fact, we can use one or many TLDs, but the result will be always the same: there will be cases where we change the PSL and we are forced to update the test file.

And what will happen if, for some reason, a test is no longer valid? For example, I had to change the

> TLD with only 1 (wildcard) rule.

test to use another TLDs with a wildcard. But I've just shifted the issue from one domain to another.

My proposal is, instead, to create a small PSL subset including some rules and test against them. We will have the same result, because we are not testing the PSL itself, but the rule definitions (for example, we are testing if a wildcard behaves as expected, not the result of that specific wilcard TLD).

For test purpose, the PSL file could even include a dummy value such as *.foo.

You may argue that in this way we leave outside a test that stresses the format of the PSL submitted to the repo. This is true. We can then add one or more tests only for that, but without relying on the content.

Comment 2

[Gervase Markham [:gerv]]

I don't want to put test data in the real PSL file ("*.foo"). That would mean people would recognise .foo as a real TLD!

I would rather the tests parsed the real PSL file. The tests have caught problems before where we made the file invalid.

I am not keen on the idea of a test_psl.dat PSL because that means that this file and the tests are a mutually-supporting pair, and they could say or test anything as long as they matched each other! A possible scenario is that we change the PSL or its format or something else about it, and the test doesn't have to change (because it doesn't use the real PSL) and so doesn't.

I think it's fine if we have to update the tests when we change the PSL. That's not a problem. The problem is forgetting to do so, which my comments would fix. I think it's a feature that we occasionally have to revisit the test file, as it helps us to keep in mind that we need to check it provides good test coverage.

Gerv

Comment 3

[Simone Carletti]

Hi Gervase,

I probably didn't explain myself.

> I don't want to put test data in the real PSL file ("*.foo"). That would mean people would recognise .foo as a real TLD!

I absolutely agree. I was suggesting about creating a test PSL file, not injecting foreign data in the existing PSL. You can point the test to the test PSL, instead of the real file.

Comment 4

[Gervase Markham [:gerv]]

(In reply to Simone Carletti from comment #3)
> I absolutely agree. I was suggesting about creating a test PSL file, not
> injecting foreign data in the existing PSL. You can point the test to the
> test PSL, instead of the real file.

OK. My other comments addressed that possibility.

Gerv

Comment 5

[Jothan Frakes]

It would not be incompatible with the root to add some zones to the PSL that were for testing.

There is latitude in RFC 2606 to add in the following 4 "test" TLDs: .EXAMPLE, .INVALID, .TEST, .LOCALHOST
as well as EXAMPLE.[COM|NET|ORG]

http://tools.ietf.org/html/rfc2606

Comment 6

[Simone Carletti]

References

https://bugzilla.mozilla.org/show_bug.cgi?id=985003
https://bugzilla.mozilla.org/show_bug.cgi?id=936829#c6

Comment 7

[Simone Carletti]

The test case is now run after each commit. It's also possible to run it locally using

```
make test
```

It's no longer necessarily to annotate the items in the example because we can get a quick feedback, as long as we pay attention to the build results and/or we take the habit to run the tests before making a commit.