Bug 64303 - REGRESSION (r90489): TestFailures page says far more tests are failing on the Leaks bot than actually are
Summary: REGRESSION (r90489): TestFailures page says far more tests are failing on the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Roben (:aroben)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 12:05 PDT by Adam Roben (:aroben)
Modified: 2011-07-11 12:21 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.29 KB, patch)
2011-07-11 12:06 PDT, Adam Roben (:aroben)
abarth: review+
aroben: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-07-11 12:05:28 PDT
REGRESSION (r90489): TestFailures page says far more tests are failing on the Leaks bot than actually are
Comment 1 Adam Roben (:aroben) 2011-07-11 12:06:14 PDT
Created attachment 100342 [details]
Patch
Comment 2 Adam Barth 2011-07-11 12:11:40 PDT
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.
Comment 3 Adam Roben (:aroben) 2011-07-11 12:17:26 PDT
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.
Comment 4 Adam Roben (:aroben) 2011-07-11 12:21:13 PDT
Committed r90778: <http://trac.webkit.org/changeset/90778>