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

Fix test surrogate script declarativeNetRequest unit test flakes #1451

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

kzar
Copy link
Collaborator

@kzar kzar commented Oct 12, 2022

The declarativeNetRequest unit tests would fail very occasionally, usually around the "surrogate script" checks. It turns out, that depending on the order the unit tests run, the shared Trackers instance would have different "lists" set and that would change the outcome of the surrogate declarativeNetRequest tests.

Generally, Trackers.supportedSurrogates would have the value:

{
  "ga.js": "data:application/javascript;base64,KGZ1bmN0aW9uKCkge30pKCk7Cmdvb2dsZS1hbmFseXRpY3MuY29tL2FuYWx5dGljcy5qcyBhcHBsaWNhdGlvbi9qYXZhc2NyaXB0CihmdW5jdGlvbigpIHt9KSgpOw==",
  "sdk.js": "data:application/javascript;base64,KCgpID0+IHsKfSkoKQ=="
}

But occasionally[1] the value would be:

{
  "tracker": "data:application/javascript;base64,KGZ1bmN0aW9uKCkge3ZhciB0cmFja2VyPXRydWV9KSgpOw==",
  "script.js": "data:application/javascript;base64,KCgpID0+IHsKICAgICd1c2Ugc3RyaWN0JzsKfSkoKTs="
}

That would cause the result to be different, since "ga.js" and "sdk.js" were no longer considered as supported "surrogate scripts" and therefore corresponding requests would be blocked instead of redirected.

While investigating this problem, I also noticed that the import of shared/js/background/trackers.es6 in 3p-cookies-tests.js was wrongly named. I have fixed that here too, since it confused the situation further.

I have run the unit tests repeatedly for 30 mins (using the handy run-one script[2]), and they seem to pass reliably now.

1 - As a result of 3-cookies-tests.js and possibly other tests loading the tds.json included with the the privacy reference tests[3].
2 - https://blog.dustinkirkland.com/2013/09/introducing-run-one-constantly-run-one.html
3 - https://github.com/duckduckgo/privacy-reference-tests

Reviewer: @kdzwinel
CC: @jonathanKingston

Steps to test this PR:

  1. If you like, run the unit tests a bunch of times. But I have already done that.

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

The declarativeNetRequest unit tests would fail very occasionally,
usually around the "surrogate script" checks. It turns out, that
depending on the order the unit tests run, the shared Trackers
instance would have different "lists" set and that would change the
outcome of the surrogate declarativeNetRequest tests.

Generally, Trackers.supportedSurrogates would have the value:

{
  "ga.js": "data:application/javascript;base64,KGZ1bmN0aW9uKCkge30pKCk7Cmdvb2dsZS1hbmFseXRpY3MuY29tL2FuYWx5dGljcy5qcyBhcHBsaWNhdGlvbi9qYXZhc2NyaXB0CihmdW5jdGlvbigpIHt9KSgpOw==",
  "sdk.js": "data:application/javascript;base64,KCgpID0+IHsKfSkoKQ=="
}

But occasionally[1] the value would be:

{
  "tracker": "data:application/javascript;base64,KGZ1bmN0aW9uKCkge3ZhciB0cmFja2VyPXRydWV9KSgpOw==",
  "script.js": "data:application/javascript;base64,KCgpID0+IHsKICAgICd1c2Ugc3RyaWN0JzsKfSkoKTs="
}

That would cause the result to be different, since "ga.js" and
"sdk.js" were no longer considered as supported "surrogate scripts"
and therefore corresponding requests would be blocked instead of
redirected.

While investigating this problem, I also noticed that the import of
shared/js/background/trackers.es6 in 3p-cookies-tests.js was wrongly
named. I have fixed that here too, since it confused the situation
further.

I have run the unit tests repeatedly for 30 mins (using the handy
run-one script[2]), and they seem to pass reliably now.

1 - As a result of 3-cookies-tests.js and possibly other tests loading
    the tds.json included with the the privacy reference tests[3].
2 - https://blog.dustinkirkland.com/2013/09/introducing-run-one-constantly-run-one.html
3 - https://github.com/duckduckgo/privacy-reference-tests
@kzar kzar requested a review from kdzwinel October 12, 2022 13:14
@kzar kzar marked this pull request as ready for review October 12, 2022 13:32
@jonathanKingston jonathanKingston merged commit b0c6f9b into duckduckgo:develop Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants