Change PSL test spec to JSON (+comments)

RESOLVED FIXED

Status

()

Core
Networking: Domain Lists
--
enhancement
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: zwol, Assigned: Simone Carletti)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-would-take])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
It is surprisingly difficult to parse the existing PSL test spec if you don't have a JavaScript interpreter to hand.  For convenience of third-party code that needs to use this test suite, I would like to suggest that we switch it to a JSON-based format instead: specifically, JSON + //-comments. Third party code, whatever language it happens to be in, can generally get at a JSON parser.

Patch to follow.
(Reporter)

Comment 1

4 years ago
Created attachment 829793 [details] [diff] [review]
936829-psl-tests-json.diff
Assignee: nobody → zackw
Status: NEW → ASSIGNED
Attachment #829793 - Flags: review?(gerv)
(Reporter)

Comment 2

4 years ago
One point of clarification: the (null, null) test is moved to the end of the file because JSON doesn't permit a trailing comma on the last element of an array, so it's nice to have the very last thing in the list be something that won't need to change.
Comment on attachment 829793 [details] [diff] [review]
936829-psl-tests-json.diff

I know at least one other person is using this file. I guess they are our tests so we can change them if we want to...

The readability gain doesn't seem to be awesome. It would be greater if we used more meaningful keys ("input" and "publicsuffix"?) and if we aligned each group creatively, matching up common parts, so it was more obvious what was being tested.

Gerv
Attachment #829793 - Flags: review?(gerv) → review-

Comment 4

4 years ago
You can use the following regex on the contents of test_psl.txt:
 checkPublicSuffix\((null|'[^']*'), (null|'[^']*')\);

It's also relatively easy to treat as normal function calls in other languages, although you may need to change the single quotes ito double for languages like C.
(Reporter)

Comment 5

4 years ago
(In reply to Ángel from comment #4)
> You can use the following regex on the contents of test_psl.txt:
>  checkPublicSuffix\((null|'[^']*'), (null|'[^']*')\);

There aren't any right now, but how do I know there will never be a \-escape in one of these string constants?
(Assignee)

Comment 6

2 years ago
I'd like to resurrect this discussion. I recently did some work to add automatic tests to the PSL and I had the same immediate reaction.

From https://github.com/publicsuffix/list/pull/123

> My next step is to convert the tests in test_psl.txt into the new test suite. Actually, I'm planning to make these tests available in a sort of json format so that they can be used by other implementations to test an input and an expected output.

I eventually realized that JSON is not the best format, as *JSON doesn't allow comments*. So what I did, what to change the list to be simpler, a simple list of input, output pairs.
See https://github.com/publicsuffix/list/blob/testsuite/test/tests.txt

I think that this file is valuable, as it represents a sort of standardized guidance for library implementations. In fact, I used it to test the compliance of my Ruby implementation.
https://github.com/weppos/publicsuffix-ruby/commit/cbc06f85850faa4b834354bb1ec2ba9b54422a7b

I also know that the Go publicsuffix package embeds these tests, as well the libpsl uses them.

My proposal is to keep it but convert it into a simpler version. We can even consider to use double quotes, as it seems to be the most common approach to determine a string if we consider the majority of programming languages.

FYI in https://github.com/publicsuffix/list/pull/123 I didn't change the original file, I renamed it and updated it to avoid blocking the merge. However, I vote to consolidate the files and remove the old one if that pull request will be merged.
Whiteboard: [necko-would-take]
(Assignee)

Updated

2 years ago
Assignee: zackw → weppos
(Assignee)

Comment 7

2 years ago
As mentioned, JSON is not the best format for a test suite because of the lack of comments.

After https://github.com/publicsuffix/list/pull/130 was merged we now have a test suite that no longer includes the function names. Each line is now either a comment or a test, tests are space-separated tuples:

```
input output
```
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.