Open Bug 366072 Opened 13 years ago Updated 11 years ago

Enhance reftest so that it can be used for nsIWebBrowserPersist tests

Categories

(Testing :: Reftest, enhancement)

enhancement
Not set

Tracking

(Not tracked)

People

(Reporter: asqueella, Assigned: asqueella)

Details

Attachments

(1 file, 1 obsolete file)

Reftest sounds like a good way to test our page-saving component in the 'Save Page Complete' mode.
Attached patch patch (obsolete) — Splinter Review
Adds support for 
 test-saving <url>
and
 test-saving-f <url>
(for expected failures)

in reftest manifests. The test is done by loading <url>, saving the document to a temporary location using the 'web page, complete' method, then loading the document from the temporary location and comparing to the original.

This patch also includes various cleanup of reftest itself (better behavior in failure conditions), I can make it a separate patch if you like.
Attachment #250620 - Flags: review?(dbaron)
the patch has a typo - "catch (ex)" should be changed to "catch (e)"
Comment on attachment 250620 [details] [diff] [review]
patch

>Index: reftest/reftest.js
+        } else if (items[0] == TEST_SAVING_COMMAND ||
+                    items[0] == TEST_SAVING_FAIL_COMMAND) {

This is mis-indented.

>+                setTimeout(function() {
>+                  try {
>+                      DocumentLoaded();
>+                  } catch(e) {
>+                      ReportError(e);
>+                  }
>+                }, 20);

This doesn't match file indentation style (4 spaces).

>Index: reftest/reftest.xul

>-    <script type="application/ecmascript" src="reftest.js" />
>+    <script type="application/x-javascript" src="reftest.js" />
>+    <script type="application/x-javascript" src="webbrowserpersist.js" />

application/ecmascript is registered and supported, and defined more strictly by RFC 4329.  (Not sure if we do the stricter handling yet, though.)

>Index: reftest/webbrowserpersist.js

>+  testSavingCurrentDocument: function()
>+  {
>+    var tempFolder = this._getTempFolder();
>+    try {
>+      var [,,name,ext] =
>+        gBrowser.currentURI.spec.match(/^(.*\/)?([^\/]+)\.([^.]+)$/);
>+    } catch(e) {
>+      throw "Could not extract leaf filename and extension from '" +
>+              url.spec + "'!";
>+    }

Why is an extension mandatory?

>+  _getTempFolder: function()
>+  {
>+    var file = CC["@mozilla.org/file/directory_service;1"]
>+               .getService(CI.nsIProperties)
>+               .get("TmpD", CI.nsIFile);
>+    file.append("mozilla-wbp-tests");
>+  
>+    if(file.exists() && this._firstIteration) {
>+      this._firstIteration = false;
>+      try {
>+        file.remove(true);
>+      } catch(e) {
>+        throw "Can't remove " + file.path + ": " + e;
>+      }
>+    }
>+    if (!file.exists()) {
>+      file.create(CI.nsIFile.DIRECTORY_TYPE, 0664);
>+    }
>+    return file;
>+  },

Shouldn't we remove the temp folder when we're done?  And isn't there a common mechanism for creating a uniquely named temporary file/directory?  (CreateUnique, perhaps?)

>+  _webProgressListener: {

A bunch of stuff in here colud be shortened with s/Components\.interfaces/CI/ and s/Components\.results/CR/.
Also, I think you should load the documents the other way around, since the convention is to do the test first and then the reference.
What have you done to test these changes?
Attachment #250620 - Attachment is obsolete: true
Attachment #250620 - Flags: review?(dbaron)
Component: Testing → Reftest
Product: Core → Testing
Version: Trunk → unspecified
QA Contact: testing → reftest
You need to log in before you can comment on or make changes to this bug.