Bug 64303 – REGRESSION (r90489): TestFailures page says far more tests are failing on the Leaks bot than actually are

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Roben (:aroben)]

REGRESSION (r90489): TestFailures page says far more tests are failing on the Leaks bot than actually are

[Adam Roben (:aroben)]

Created attachment 100342 [details]
Patch

[Adam Barth]

Comment on attachment 100342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100342&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/Builder_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/Builder_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/Builder_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.

[Adam Roben (:aroben)]

Comment on attachment 100342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100342&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/Builder_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.

I'll think about adding something like that in a future patch.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/Builder_unittests.js:37
>> +    };
> 
> 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.

I agree this isn't great. TestFailures hasn't really been designed with testing in mind so far. Hopefully I'll be able to improve its testability as I make other enhancements.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/Builder_unittests.js:45
>> +    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.

OK.

[Adam Roben (:aroben)]

Committed r90778: <http://trac.webkit.org/changeset/90778>