Conversation
cunha
left a comment
cunha
left a comment
There was a problem hiding this comment.
Main points:
- Tests don't match expected behavior; we need to define a number of domain and path levels beyond which we start "anonymization"
- Need tests to cover URL query parameters
- Need tests to cover special domains and CC TLDs
cunha
left a comment
cunha
left a comment
There was a problem hiding this comment.
Forgot to mention CC TLDs in the previous review.
cunha
left a comment
cunha
left a comment
There was a problem hiding this comment.
Another round of comments. I think we should focus on comparing the URL identity keys (after we "erase" subdomains, path levels, and query parameter values). The filter is just taking one URL for each distinct identity key.
| """Testing a single URL identity key.""" | ||
| url = "http://example.com/unique" | ||
| result = filter_instance.anonymize_url(url) | ||
| expected = "n1/unique" |
There was a problem hiding this comment.
I think this should be http://example.com/unique, no? Why the n1?
There was a problem hiding this comment.
Looking at the other tests, it seems like we are really anonymizing the URLs, but we do not need to (and should not) anonymize in this project, as we need to know exactly what the URL is to crawl it later.
| "http://sub.example.com/page", | ||
| "http://another.example.com/page" | ||
| ] | ||
| result = filter_instance.filter_urls(urls) |
There was a problem hiding this comment.
We should focus our tests on anonymize_urls, to check what URL identity keys are being generated. The filter is then just running set() on the list of anonymized URLs.
|
|
||
| def test_special_cctld(self, filter_instance): | ||
| """Test URL with special ccTLD identity key.""" | ||
| url = "http://example.co/page" |
There was a problem hiding this comment.
We need tests for long subdomains like a.b.c.d.com.br to make sure we're erasing the correct subdomains according to the config.
2 participants
No description provided.