Closed Bug 1022725 Opened 7 years ago Closed 7 years ago

Create a mock httpd.js Translation provider for tests

Categories

(Firefox :: Translation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: Felipe, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: p=8 s=33.1 [qa-])

Attachments

(2 files, 5 obsolete files)

We should create a mock provider for tests using httpd.js that runs during the translation tests and replies to translation/token requests correctly so that BingTranslator can be tested and not access the network.
Flags: firefox-backlog+
Blocks: 971054
Whiteboard: p=0 [qa?]
Whiteboard: p=0 [qa?] → p=8 [qa?]
Whiteboard: p=8 [qa?] → p=8 [qa-]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Whiteboard: p=8 [qa-] → p=8 s=33.1 [qa-]
Florian, requesting a round of feedback first, even though I *think* this is very close to what we want.

What do you think?
Attachment #8439826 - Flags: feedback?(florian)
Attachment #8439826 - Attachment is obsolete: true
Attachment #8439826 - Flags: feedback?(florian)
Attachment #8439829 - Flags: feedback?(florian)
Comment on attachment 8439829 [details] [diff] [review]
Patch v1: add a mock httpd.js Translation provider for tests

Review of attachment 8439829 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to be going in a good direction. I haven't fully reviewed the server code (it seems unfortunate that the file I/O / stream code is so verbose, but I don't see obvious ways to write it more concisely).

::: browser/components/translation/BingTranslator.jsm
@@ +249,5 @@
>        let auth = "Bearer " + token;
> +      let url = "https://api.microsofttranslator.com/v2/Http.svc/TranslateArray";
> +      try {
> +        url = Services.prefs.getCharPref("browser.translation.bing.translateArrayURL");
> +      } catch (ex) {}

I'm not excited by this code that will run and cause (caught) exceptions outside of the tests.

Here is a suggestion for another way it could be done (it has its downsides too):
- set translateArrayURL and authURL in BingTranslation.prototype
- make the BingRequest constructor take as parameter a BingTranslation instance, instead of the current sourceLanguage and targetLanguage parameters.
- in your test, between |let client = new BingTranslation(...);| and |client.translate()...|, override client.translateArrayURL and client.authURL.

::: browser/components/translation/test/browser_translation_bing.js
@@ +38,5 @@
> +    if (tab.linkedBrowser.currentURI.spec == "about:blank")
> +      return;
> +
> +    tab.linkedBrowser.removeEventListener("load", onload, true);
> +    Task.spawn(function* () {

I think you either missed a 'yield' somewhere, or you don't really need a task.

@@ +53,5 @@
> +      } catch (ex) {
> +        ok(false, "Unexpected Exception: " + ex);
> +      }
> +    });
> +   }, true);

nit: indent doesn't seem right on this line.
Attachment #8439829 - Flags: feedback?(florian) → feedback+
Attachment #8439829 - Attachment is obsolete: true
Attachment #8439871 - Flags: review?(florian)
Comment on attachment 8439871 [details] [diff] [review]
Patch 1.1: rename BingTranslation to BingTranslator and allow overriding URLs

Review of attachment 8439871 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/BingTranslator.jsm
@@ +238,5 @@
>    this.translationData = translationData;
> +  this.sourceLanguage = bingTranslator.sourceLanguage;
> +  this.targetLanguage = bingTranslator.targetLanguage;
> +  this.translateArrayURL = bingTranslator.translateArrayURL;
> +  BingTokenManager.authURL = bingTranslator.authURL;

Wouldn't this cause BingTokenManager to be in a different state after the tests are done running?

Maybe we could store authURL here, and give the url as a parameter to the getToken method?
Or we can just put the urls in a pref in firefox.js? What's the downside of that?
(In reply to Mike de Boer [:mikedeboer] from comment #7)
> Or we can just put the urls in a pref in firefox.js? What's the downside of
> that?

Can we think of any reason why users would want to change these urls?
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Can we think of any reason why users would want to change these urls?

Hmmz... good point! :)
From a conversation on IRC, we concluded it's better to go back to the first approach for the pref. That way, we can set the URL prefs in the test harness code (instead of any single individual test), and be assured that no test will accidentally trigger BingTranslator and end up using our keys and quota during the test.

You can even reuse the getAuthTokenParam (rename it again? :S) to avoid writing the try { } catch again in two new places.

If you feel very compelled to remove the exception you can use S.prefs.getPrefType to check for its existence
Comment on attachment 8439871 [details] [diff] [review]
Patch 1.1: rename BingTranslation to BingTranslator and allow overriding URLs

Review of attachment 8439871 [details] [diff] [review]:
-----------------------------------------------------------------

Felipe has good points in comment 10.
Attachment #8439871 - Flags: review?(florian) → review-
Comment on attachment 8439874 [details] [diff] [review]
Patch 2: add a mock httpd.js Translation provider for tests

Review of attachment 8439874 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/test/fixtures/bug1022725-en.txt
@@ +18,5 @@
> +    <TranslatedTextSentenceLengths xmlns:a="http://schemas.microsoft.com/2003/10/Serialization/Arrays">
> +      <a:int>475</a:int>
> +    </TranslatedTextSentenceLengths>
> +  </TranslateArrayResponse>
> +</ArrayOfTranslateArrayResponse>

Unfortunately I think we can't land the translated text provided by the service in the tree. So we should change this to some Lorem Ipsum gibberish..
Comment on attachment 8439874 [details] [diff] [review]
Patch 2: add a mock httpd.js Translation provider for tests

Review of attachment 8439874 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/test/bing.sjs
@@ +11,5 @@
> +
> +function handleRequest(req, res) {
> +  try {
> +    reallyHandleRequest(req, res);
> +  } catch (ex) {

Do we actually expect this code path to be used?

@@ +22,5 @@
> +
> +function log(msg) {
> +  // if (!msg)
> +    return;
> +  dump("BING-SERVER-MOCK: " + msg + "\n");

What about just commenting out the dump line, and removing the other 2 lines before?

@@ +115,5 @@
> +    return sendError(res, "Bing only deals with POST requests, not '" +
> +                          req.method + "'.");
> +  }
> +
> +  let body = getRequestBody(req);

I think I would find the code flow here easier to follow if all the small helper functions that are called only once (parseQuery, getRequestBody, getXml, checkAuth) were inlined; but it's likely just a matter of personal preference.

::: browser/components/translation/test/browser_translation_bing.js
@@ +48,5 @@
> +      } catch (ex) {
> +        ok(false, "Unexpected Exception: " + ex);
> +      }
> +    });
> +   }, true);

The indent still doesn't look right on this line.
Bug 973292 added a disabled test that should be enabled with the mock server
Blocks: 973292
Attachment #8439871 - Attachment is obsolete: true
Attachment #8439874 - Attachment is obsolete: true
Attachment #8439874 - Flags: review?(florian)
Attachment #8442128 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> @@ +115,5 @@
> > +    return sendError(res, "Bing only deals with POST requests, not '" +
> > +                          req.method + "'.");
> > +  }
> > +
> > +  let body = getRequestBody(req);
> 
> I think I would find the code flow here easier to follow if all the small
> helper functions that are called only once (parseQuery, getRequestBody,
> getXml, checkAuth) were inlined; but it's likely just a matter of personal
> preference.

I did it this way because, eventually, I'd like to see some sort of shared utility library for .sjs files. I already had to inspect a dozen or so existing files to pick the best version(s) of this code for getting an input stream, fetching the request body, query parsing, instantiating an XML parser, etc.
Normally you'd find these functions in a shared library, but we don't have that yet.

Personally I also think that 'lightweight' function bodies usually yield a better readability and maintainability of the code in question. But, like you said, this is most likely a matter of personal preference.
I'm not sure if this file is used for all test suites, but if it does (or at least for b-c), can you define the auth and translation URLs directly here instead of in the test:
http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js

that way we can rest assured that no tests will be hitting Bing's servers.
Attachment #8442128 - Attachment is obsolete: true
Attachment #8442128 - Flags: review?(florian)
Attachment #8442348 - Flags: review?(florian)
Comment on attachment 8442126 [details] [diff] [review]
Patch 1.2: rename BingTranslation to BingTranslator and allow overriding URLs

Review of attachment 8442126 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/BingTranslator.jsm
@@ +422,5 @@
>   * Fetch an auth token (clientID or client secret), which may be overridden by
>   * a pref if it's set.
>   */
> +function getUrlParam(paramValue, prefName, encode = true) {
> +  if (Services.prefs.getPrefType(prefName))

I would vaguely prefer checking != Ci.nsIPrefBranch.PREF_INVALID or == Ci.nsIPrefBranch.PREF_STRING, but it seems there are already a few places that just check getPrefType returns a truthy value like you do, so I don't care strongly.
Attachment #8442126 - Flags: review?(florian) → review+
Comment on attachment 8442348 [details] [diff] [review]
Patch 2.2: add a mock httpd.js Translation provider for tests

Review of attachment 8442348 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/test/bing.sjs
@@ +96,5 @@
> +  // Convert the binary hash data to a hex string.
> +  return [toHexString(hash.charCodeAt(i)) for (i in hash)].join("");
> +}
> +
> +function getXml(body) {

would 'parseXml' be more explicit than 'getXml' about what the function does?

@@ +113,5 @@
> +  for (let part of path.split("/"))
> +    file.append(part);
> +  let fileStream  = Cc["@mozilla.org/network/file-input-stream;1"]
> +                      .createInstance(Ci.nsIFileInputStream);
> +  fileStream.init(file, 1, 0, false);

The meaning of these 2 constants isn't obvious.
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIFileStreams.idl#17 says to use -1 for default values.

But it also seem the values you used are the actual defaults (1 is http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prio.h#588 (PR_RDONLY       0x01)).

@@ +133,5 @@
> +function reallyHandleRequest(req, res) {
> +  log("method: " + req.method);
> +  if (req.method != "POST") {
> +    return sendError(res, "Bing only deals with POST requests, not '" +
> +                          req.method + "'.");

This will cause a "function doesn't always return a value" JS strict warning, please put |return;| on a separate line.

@@ +176,5 @@
> +const methodHandlers = {
> +  authenticate: function(res, params) {
> +    // Validate a few required parameters.
> +    if (params.scope != "http://api.microsofttranslator.com")
> +      return sendError(res, "Invalid scope.");

These 3 returns will also cause warnings.

::: browser/components/translation/test/browser_translation_bing.js
@@ +31,5 @@
> +    Services.prefs.clearUserPref(kClientIdPref);
> +    Services.prefs.clearUserPref(kClientSecretPref);
> +  });
> +
> +  tab.linkedBrowser.addEventListener("load", function onload() {

tab.linkedBrowser is used 4 times, I would put it in a temporary variable.
Attachment #8442348 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/70abfc86cba6
https://hg.mozilla.org/mozilla-central/rev/9e8c00218dcc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment on attachment 8442126 [details] [diff] [review]
Patch 1.2: rename BingTranslation to BingTranslator and allow overriding URLs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug is part of the automatic
translation feature, which we want to A/B with a subset of Aurora 32 users.
User impact if declined: This patch changes the exported symbol from `BingTranslation` to `BingTranslator`, which means that future uplifts are based on this interface.
Testing completed (on m-c, etc.): baked on m-c for several days.
Risk to taking this patch (and alternatives if risky): minor.
String or IDL/UUID changes made by this patch: n/a.
Attachment #8442126 - Flags: approval-mozilla-aurora?
Comment on attachment 8442348 [details] [diff] [review]
Patch 2.2: add a mock httpd.js Translation provider for tests

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug is part of the automatic
translation feature, which we want to A/B with a subset of Aurora 32 users.
User impact if declined: This patch adds a robust testing part for the Bing service that will be used for unit tests developed in the (near) future.
Testing completed (on m-c, etc.): baked on m-c for several days.
Risk to taking this patch (and alternatives if risky): minor.
String or IDL/UUID changes made by this patch: n/a.
Attachment #8442348 - Flags: approval-mozilla-aurora?
Attachment #8442126 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8442348 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.