[webkit-reviews] review granted: [Bug 64303] REGRESSION (r90489): TestFailures page says far more tests are failing on the Leaks bot than actually are : [Attachment 100342] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Adam Roben (:aroben)
<aroben at apple.com>'s request for review:
Bug 64303: REGRESSION (r90489): TestFailures page says far more tests are
failing on the Leaks bot than actually are
https://bugs.webkit.org/show_bug.cgi?id=64303

Attachment 100342: Patch
https://bugs.webkit.org/attachment.cgi?id=100342&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100342&action=review


>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/Builde
r_unittests.js:29
> +	   successCallback(mockXHR);

This will happen synchronously, rather than async, which might change the code
under test.

NetworkSimulator (in the other patch you reviewed) is another approach to
creating a mock network.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/Builde
r_unittests.js:37
> +    var realPersistentCache = window.PersistentCache;
> +    window.PersistentCache = {
> +	   contains: function() { return false; },
> +	   set: function() { },
> +	   get: function() { },
> +    };

Generally, poking at a few global objects to mock them out is fragile because
changes to the code under test can introduce more real dependencies, e.g., on
the network.  One way to solve this problem is to refactor the code under test
to go through a single mockable object, which serves as a choke point.	I need
to refactor garden-o-matic to have a dedicated module for that instead of
conflating it with the base module.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/Builde
r_unittests.js:45
> +    window.getResource = realGetResource;
> +    window.PersistentCache = realPersistentCache;

I'd also add some equals tests to make sure you execute these lines of code,
otherwise you can get into trouble with cascading failures where your mocks
leak from one test to another.