Closed Bug 1088116 Opened 11 years ago Closed 11 years ago

Marionette support for testing l10n builds at parity with mozmill

Categories

(Testing :: General, defect)

Product:
Testing
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

I'm not sure what the scope of this will end up being, but I wanted some place to document this a bit. According to :whimboo, the relevant functions used by these tests are getEntity and getProperty in http://hg.mozilla.org/qa/mozmill-tests/file/d5b1d70f8c86/lib/utils.js
The question here is if this should go into Marionette core or if it should stay in our own library modules for Firefox tests. Given that this would be supported across platforms, it would even work on b2g, where we use the same technique, right?
Please also see bug 1088007 where we extend getProperty to also accept a list of URLs. That is for a unique consistency between those two methods. To explain for what those are used: * getEntity retrieves a localized string for the given placeholder in a XUL file * getProperty retrieves a localized string for a given property as used in js files to dynamically change e.g. a label
(In reply to Henrik Skupin (:whimboo) from comment #1) > The question here is if this should go into Marionette core or if it should > stay in our own library modules for Firefox tests. Given that this would be > supported across platforms, it would even work on b2g, where we use the same > technique, right? I don't think I have the answer to this question (I'll make it a topic for Monday's meeting if we don't have an obvious answer here), but from what I've seen we might have a category of things that are functions general enough to be used by multiple mozmill tests now, but not of interest for the marionette client itself. It's not clear to me where those would go in relation to the repositories created for the effort in bug 1080772, but it looks like they could belong in firefox-puppeteer. Also it looks like we are preferring python for implementing the tests, so a python interface to most of these will probably make sense.
Just to be clear here, marionette-core usually means the marionette server written in JS. I think we're talking about the marionette python client. I don't think we want anything that is Firefox specific being included in marionette-client. There is a bit of B2G specific stuff in there, but there are bugs on file to get rid of it. Anything that is Firefox specific should go in firefox-puppeteer. The way I see it, there are three categories of functionality: 1) Stuff generally applicable to any gecko product -> lives in marionette-client 2) Stuff generally applicable to Firefox -> lives in firefox-puppeteer 3) Stuff only applicable to this particular test suite -> lives alongside the tests
Just a quick note given that everything else mentioned here including a possible firefox-puppeteer repository is talked about in the other bug... The code we have for retrieving localized content does not only include shortcuts, but also any kind of text you see in the ui including the menu, button labels etc. It can be done via getEntity() and getProperty(). I'm not sure about Firefox OS and Mobile, but any other application (Thunderbird, SeaMonkey, etc.) is also supported by this method. So this feature will be all Gecko.
Ok, so I got the initial l10n lib working, which contains two methods for retrieving the value of a DTD entity and a property. The changes landed as: https://github.com/mozilla/firefox-greenlight-tests/commit/6e1b948f1b7f2c3c548f8781c64ae5d14f9700ec Something which is strange, I cannot run the test lib/tests/l10n.py. It works when I specify the manifest bug no test is actually run by Marionette, and it returns with 0 for pass, fail, and skip. Chris can you have a look at this while I continue the work on other tests? Also I think this bug should be owned by you, when I'm not totally wrong.
Assignee: nobody → cmanchester
Flags: needinfo?(cmanchester)
I think the issue is the test file name. Renaming to test_l10n.py and changing the name in the manifest makes these run and pass for me locally (pushed the change to master). The marionette runner does a check before adding files to the test set: http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#714
Flags: needinfo?(cmanchester)
I went to convert a simple test that uses one or another of these functions but many depend on modifiers for key events. :whimboo, I think I saw you asking about this yesterday, do we need to file a bug to add this to marionette? A little digging made me think it's not supported at the moment.
Flags: needinfo?(hskupin)
Exactly that is next on my list. I just wanted to get out the l10n module so that you can see how it will work, and figure out if we want this in marionette client or not. Maybe we should also get David on board on this bug for a solution finding. David, can you also please have a look at the commit as posted in comment 6? Those two methods can be used to retrieve localized strings from Gecko applications. We wonder if those could be part of Marionette client itself given that nearly all apps should suppport that.
Flags: needinfo?(hskupin) → needinfo?(dburns)
We have the concept that you find an element and then get back it's text. marionette.find_element(<location strategy>, <search term>).text [1] We return back normalised text and apply our visibility(at least on content and we should on Chrome if doesnt) algorithm so that you get the text that is visible to the user. [1] http://marionette-client.readthedocs.org/en/latest/#marionette.HTMLElement.text Hope that helps
Flags: needinfo?(dburns)
(In reply to David Burns :automatedtester from comment #10) > We have the concept that you find an element and then get back it's text. > > marionette.find_element(<location strategy>, <search term>).text [1] Ok, so in such a case you won't be able to test the UI if the correctly translated text is shown. That is exactly what we do in a couple of cases for Mozmill tests. Also in some cases for Firefox Desktop you can only get an element with its label. Given that we do not know that label (each locale is different), we would not be able to retrieve it at all. This applies for example to some notification bar elements like inside the geolocation or safebrowsing notification.
(In reply to Chris Manchester [:chmanchester] from comment #7) > I think the issue is the test file name. Renaming to test_l10n.py and > changing the name in the manifest makes these run and pass for me locally > (pushed the change to master). The marionette runner does a check before > adding files to the test set: > http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/ > marionette/runner/base.py#714 I was mistaken, that's not where the filtering occurs. It's based on the regex in the class variable here: http://hg.mozilla.org/mozilla-central/file/f10c312a4bd9/testing/marionette/client/marionette/marionette_test.py#l476 Overriding the class variable in FirefoxTestCase (match_re = re.compile(r'.*'), or whatever we like) works fine.
Ah I see. So no it's fine then. Thanks for fixing it.
Not sure if I do something wrong here in the test. After defining the encoding of the file as utf-8 and running this test I get a failure. test code: > value = self.marionette.execute_script("""return 'موزیلا';""") > self.assertEqual(value, 'موزیلا') result: > AssertionError: u'\u0645\u0648\u0632\u06cc\u0644\u0627' != '\xd9\x85\xd9\x88\xd8\xb2\xdb\x8c\xd9\x84\xd8\xa7' David, I assume execute_script somewhat messes around with the encoding, right?
Flags: needinfo?(dburns)
Ups my fault. It should have been u'موزیلا'. All fine here.
Flags: needinfo?(dburns)
At least get_localized_entity is working ok in the shortcuts test and as far as I got on the private browsing test. I ran across some fairly involved functions in mozmill-tests/lib/localization.js. Should we consider converting these and the tests that use them a part of this bug?
Flags: needinfo?(hskupin)
This is something larger and way more complex, which I would not tackle right now. We should wait a bit until the other major bugs are fixed. Also the code in this localization library might need a larger rework, so it requires some more thinking and discussions with the l10n team.
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #17) > This is something larger and way more complex, which I would not tackle > right now. We should wait a bit until the other major bugs are fixed. Also > the code in this localization library might need a larger rework, so it > requires some more thinking and discussions with the l10n team. Ok, so long as we're clear about the scope here: this bug is about running tests of arbitrary features on localized builds, not running the mozmill tests that are specifically concerned with localization. If that's the case we're talking about the two helper functions :whimboo implemented in comment 6. I converted a full test case (hacking around bug 1094246) that uses get_localized_entity on my "pb" branch at https://github.com/chmanchester/firefox-greenlight-tests/ I think we can close this bug once we convert a full test using get_localized_property and we're convinced these tests provide coverage equivalent to the originals.
Correct, this is not about the l10n testrun. All what we need to run the tests on different locales are the methods to get the value of an DTD entity or property. But something we should do before closing this bug is to find an element by label, e.g. get the translated string for a menu entry and retrieve that element. If that works we should be fine here, and I don't see that anything else is missing. It might be good to get all this tested with an UTF-8 (Chinese), and an RTL (fa) locale.
I've been working on converting testSafeBrowsingNotificationBar.js for its use of getProperty, but I've run into an issue where navigating to the unsafe urls in the test via marionette doesn't result in the safe browsing pages. I don't know how any of this works, but somehow marionette's js is too trustworthy to be blocked. Neither calling marionette.navigate() nor sending keys to type into the urlbar and clicking go result in the overlays being shown. :whimboo, do you happen to know if we've run into issues in the past with these tests, or what might be going wrong with marionette's way of synthesizing these events vs. mozmill's? Thanks!
Flags: needinfo?(hskupin)
You have chosen one of the most complicated elements to work with Chris! You cannot get this button via an id directly. You have to go over different elements. See: http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/tabs.js#l723 http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/tabs.js#l920 So this would need the anonymous node support to get to.
Flags: needinfo?(hskupin)
From looking at things as the test was running, it seems like the page with the button on it isn't shown when marionette does the navigation, and it goes straight to the unsafe destination. I'll look into this some more today.
(In reply to Chris Manchester [:chmanchester] from comment #23) > From looking at things as the test was running, it seems like the page with > the button on it isn't shown when marionette does the navigation, and it > goes straight to the unsafe destination. I'll look into this some more today. Ah, the issue is that the SafeBrowsing.jsm adds the example pages to the db later than expected, and after the test runs. We should probably wait for this explicitly, but adding a delay works fine for now.
I took this a little further on the branch linked in comment 21. Getting buttons by their label and clicking on them is working fine, but it seems like converting the rest of this test will depend on getting marionette to interact with newly opened tabs, which I think will depend on bug 941749.
Good to see that this is working! I have seen this missing feature for prefs before the workweek. Nice that you covered it. In the following some questions to Chris: * Do we have to limit the interfaces for get complex pref to those you have listed? If yes, why? * Why do we need a _get_complex_pref method? Can't we simply handle that all in the same execute_script block as what we do in our current test code for mozmill tests? * Personally I would prefer to pass in the arguments to execute_script and not add them via a formatting with %. We need consistency. Chris, just an additional hint regarding styles and indentation. Not sure which editor you are using, but with Sublime I'm happy to have the "Python Flake8 Lint" plugin. It helps to have correct formatting. Also, I would appreciate if we could agree on to rebase (squash) all the commits against master before merging them. Having dozen of commit messages in the history makes it very hard to go back and find regressors, especially if we might have partial commits with non-working code.
OS: Mac OS X → All
Hardware: x86 → All
Thanks for the feedback. I'll clean this up and open a pull request, we can do something like a review there. (In reply to Henrik Skupin (:whimboo) from comment #27) > Good to see that this is working! I have seen this missing feature for prefs > before the workweek. Nice that you covered it. > > In the following some questions to Chris: > > * Do we have to limit the interfaces for get complex pref to those you have > listed? If yes, why? Yes, getComplexValue throws if any interface but these are specified. > * Why do we need a _get_complex_pref method? Can't we simply handle that all > in the same execute_script block as what we do in our current test code for > mozmill tests? I guess we could, but we would need to resolve the issue with nsIIDRef below. > * Personally I would prefer to pass in the arguments to execute_script and > not add them via a formatting with %. We need consistency. I agree it's hacky, but this is a way to get a nsIIDRef from python to js. > > Chris, just an additional hint regarding styles and indentation. Not sure > which editor you are using, but with Sublime I'm happy to have the "Python > Flake8 Lint" plugin. It helps to have correct formatting. I usually use pyflakes but I don't think I ran it over this code. Standards deviate between projects, so let's talk about individual issues in review. > > Also, I would appreciate if we could agree on to rebase (squash) all the > commits against master before merging them. Having dozen of commit messages > in the history makes it very hard to go back and find regressors, especially > if we might have partial commits with non-working code. Agreed, I think it's best to have commits that are each working states. There's only one commit on this branch, the one linked in comment 26.
The discussion of landing that test in the greenlight repository moved to https://github.com/mozilla/firefox-greenlight-tests/pull/30. We can take care of any follow up there.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.