Mozmill Endurance test for loading a SWF video via URL

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ashughes, Assigned: daniela.p98911)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox20 fixed, firefox21 fixed, firefox22 fixed, firefox23 fixed, firefox-esr17 fixed)

Details

(Whiteboard: [mozmill-endurance], URL)

Attachments

(7 attachments, 7 obsolete attachments)

3.39 KB, patch
ashughes
: review+
davehunt
: feedback+
Details | Diff | Splinter Review
680.95 KB, video/x-flv
Details
729.06 KB, application/x-shockwave-flash
Details
1.72 MB, patch
davehunt
: review+
Details | Diff | Splinter Review
3.72 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
3.78 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
3.78 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Create a Mozmill Endurance test for loading an SWF video by URL typed in the location bar.
Assignee: nobody → vlad.maniac
Status: NEW → ASSIGNED

Comment 1

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/16294931
Created attachment 548746 [details] [diff] [review]
WIP v1.0

Adding first WIP patch. Asking for f? from Dave, since he's owner for endurance tests. 

Now popping the big questions: 

1. Do we need to play the flash video?  
2. How do we check that the flash content loads? 
3. Do we test for both .swf and .flv files in the same test? For me it makes sense to test .flv only in embedded flash content
Attachment #548746 - Flags: feedback?(dave.hunt)
1. Ideally we'd start the video playing.
2. If we're able to start the video playing then this would fail if the flash content hasn't loaded.
3. No, check these in separate tests.

We also should use micro-iterations to load the content in multiple tabs.

Start iteration
Start micro-iteration
Open new tab (if current micro-iteration > 1)
Load Flash
End micro-iteration
End iteration
Attachment #548746 - Flags: feedback?(dave.hunt) → feedback-
"Open new tab (if current micro-iteration > 1)" are you sure we need this check? 

      var microIterationsCount = enduranceManager.microIterations; 
      if (microIterationsCount > 1) {
	tabBrowser.openTab();
         ....
(Reporter)

Comment 5

7 years ago
Dave, can you give Vlad feedback on comment 4?
I'm not sure I understand the question in comment 4. Could you clarify?

The reason for the if statement is that we don't need to open a new tab for the first time the video is loaded -- we will already have a tab open.

Also, please note that I have just landed some changes to the endurance.js shared module that changes micro-iterations to entities.

  if (enduranceManager.currentEntity > 1) {
    tabBrowser.openTab();
  }
  ....
Created attachment 558757 [details] [diff] [review]
patch v1.0 all branches

Added initial patch with requested changes
Attachment #548746 - Attachment is obsolete: true
Attachment #558757 - Flags: review?(dave.hunt)
Comment on attachment 558757 [details] [diff] [review]
patch v1.0 all branches

> diff --git a/tests/endurance/testFlash_UrlVideo/test1.js b/tests/endurance/testFlash_UrlVideo/test1.js

Should we capitalise URL in the folder name? Question on style for Henrik/Anthony.

> +function testFlashUrl() {

As above, should we capitalise URL?

> +    enduranceManager.loop(function () {
> +      // If micro-iterations number > 1 open flash content in a new tab 

Rename micro-iterations to entities. Also, this if statement should just cover opening the tab, as the content is loaded regardless.

> +      if (enduranceManager.currentEntity > 1) {
> +	tabBrowser.openTab();

Use spaces for indentation.

> +        enduranceManager.addCheckpoint("Load a web page with .swf flash content");
> +        controller.open(TEST_PAGE);
> +        controller.waitForPageLoad();
> +        enduranceManager.addCheckpoint("Web page has been loaded");

We just need to open the new tab, the loading if the content will be taken care of outside the if statement.

> +      } 
> +      else {
> +        // Load the test page in the currently opened tab

No need for an else, just open the test page.
Attachment #558757 - Flags: review?(dave.hunt) → review-
Created attachment 558793 [details] [diff] [review]
patch v1.1 all branches

Nits fixed
Attachment #558757 - Attachment is obsolete: true
Attachment #558793 - Flags: review?(dave.hunt)
Went with Henrik's advice and capitalized function name with 'URL' but not the filename. Hope its alright
Anthony, what do you think?
Attachment #558793 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 12

7 years ago
Comment on attachment 558793 [details] [diff] [review]
patch v1.1 all branches

Vlad, I would prefer acronyms to be ALLCAPS everywhere. I would also like to call out the filetype in the name (SWF in this case).

I'm not sure the best way to handle this, though here is my thought:
testFlash_SWFVideoURL -- testSWFVideoViaURL()

I'll defer to Dave on this one though.

>+      // Load the test page in the currently opened tab
>+      enduranceManager.addCheckpoint("Load a web page with .swf flash content");
>+      controller.open(TEST_PAGE);
>+      controller.waitForPageLoad();
>+      enduranceManager.addCheckpoint("Web page has been loaded"); 

Does plugin content load before or after pageLoad fires? Is plugin content included in pageLoad()?
Attachment #558793 - Flags: review?(anthony.s.hughes) → review-
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #12)
> Does plugin content load before or after pageLoad fires? Is plugin content
> included in pageLoad()?

No, it's not. There will be another event for this particular object. So just call waitForPageLoad for the plugin's document. That should work.
The test is looking good to me. Could you address Henrik's suggestion for waiting for the plugin content to load. I have little experience here so please connect with Henrik if you have any questions.

I'm a little concerned that when running I have seen timeouts locally. It appears that the first time we load the URL, the SWF file is downloaded, and at 4.4MB this can take longer than 60 seconds depending on the user's connection. I'm not sure what we should do to address this? Increase the timeout, or source a smaller video file?

Any suggestions Anthony/Henrik?

Just one minor nit:

> +      // If entities number > 1 open flash content in a new tab 

Can we reword this to "If entity > 1 then open a new tab" as the loading of the content is already commented below.
I assume this SWF doens't allow streaming? We should modify it accordingly, so we don't have to wait until it has been finished loading.
(Reporter)

Comment 16

7 years ago
FYI, all plugin content, whether local or remote, has to be loaded into the container every time the page loads. I believe we just cache it after the first time. If it is taking a long time to load 4.4MB of flash content, I think that's a valid Firefox performance concern.
The question here is why don't we use a lightweight SWF player which internally caches the Flash movie? That way the page can be loaded kinda fast and we wouldn't have issues with timeouts. To retrieve performance related data we probably could get the current cache state from the player. At the same time we could add delays to let the memory get filled up with the movie content. 4MB is quite a lot for some users.
Comment on attachment 558793 [details] [diff] [review]
patch v1.1 all branches

>+const TEST_DOMAIN = "http://www.mozqa.com/";
>+const TEST_PAGE = TEST_DOMAIN + "data/firefox/plugins/flash/sample-swf-video.swf";

I like the way as I have seen it by our Selenium tests with BASE_URL. It's something we also could specify from the command line when calling the testrun scripts. For making that happen the TEST_DOMAIN should be renamed to BASE_URL and has to contain "http://www.mozqa.com/data/firefox". So the page path will be the same for our local test files and the remove files on mozqa.com.

I will file a bug for that new feature which sounds kinda helpful.
Do we have any update on the decisions yet to be made on this bug?
(Reporter)

Comment 20

7 years ago
So there are a couple issues here...

1) SWF video takes >60s on some systems to load -- timeout or smaller video?
 * sourcing a new video will take a while and puts this test at risk for this quarter
 * can we accept a higher timeout value?

2) TEST_DOMAIN vs BASE_URL
 * changing this should fall into the refactoring bucket

Henrik, Dave, please chime in...
We also need to address comment 13.

I'm personally fine with a higher timeout being set for the short term, and perhaps a smaller video in the mid term.

I agree that setting a base URL should be a later refactoring.
(Reporter)

Comment 22

7 years ago
(In reply to Dave Hunt (:davehunt) from comment #21)
> We also need to address comment 13.
> 
> I'm personally fine with a higher timeout being set for the short term, and
> perhaps a smaller video in the mid term.
> 
> I agree that setting a base URL should be a later refactoring.

As the curator/architect for Endurance tests, Dave, is there a timeout value which would be acceptable to you?
What if the video was all black? Wouldn't that greatly reduce the size?
I would increase the timeout to 60 seconds.
(Reporter)

Comment 25

7 years ago
Please just update the timeout as Dave has requested. Any changes we decide to make to the video will not require changes to the test. Given the time it took us to secure the current video, I would not want to block this test on that process again.
Please also take into account that we probably have to update the global timeout in the EnduranceTestRun class as well.
The global timeout in EnduranceTestRun should be fine as it is. It's the value of the endurance delay plus 60 seconds (so defaults to 60.1 seconds).
Well, the delay you are referring to is not the timeout we are talking here. The delay served by the command line option is the global timeout and can be anything defined by the user. It will kill the browser. The timeout we have here is the one we are using in the test to wait for the video finished loading. It's a different one and always has to be smaller than the global timeout.
As I explained, it is smaller than the global timeout if we use 60 seconds (or less), as the global timeout is 60 seconds plus the value from the command line, which defaults to 0.1 seconds.
No, don't on that. You always have jitter in between the timer ticks. So specifying 60s timeout for waitFor doesn't mean you will have exactly 60s.
Please make a suggestion then. I'm happy with a 50 second timeout on the page load if that means we don't need to adjust the global timeout.
Sounds fine with me.
Created attachment 563414 [details] [diff] [review]
patch v1.2 all branches [backed-out]

Addressed required changes 

* filename will be testFlash_SWFVideoURL the first suggestion Anthony gave - we have to be consistent with the format of all the other files
* manually added 50 sec timeout for page load - waitForPageLoad(TIMEOUT_PAGE)
Attachment #558793 - Attachment is obsolete: true
Attachment #563414 - Flags: review?(anthony.s.hughes)
Attachment #563414 - Flags: feedback?(dave.hunt)
Attachment #558793 - Flags: review?(dave.hunt)
Comment on attachment 563414 [details] [diff] [review]
patch v1.2 all branches [backed-out]

Does the timeout need to be a constant if it's only used once in the test? I'm fine with that if Anthony & Henrik are.

In comment 12 Anthony asked if waiting for the page load is enough to ensure the plugin content has loaded. In comment 13 Henrik said that it is not, and that we'd also need to wait for the plugin content's document. As far as I can tell this hasn't been added, unless I'm missing something here?
Attachment #563414 - Flags: feedback?(dave.hunt) → feedback+
If we load the SWF file directly we probably don't have to wait for the plugin content to be loaded.
(Reporter)

Updated

7 years ago
Attachment #563414 - Flags: review?(anthony.s.hughes) → review+
(Reporter)

Comment 37

7 years ago
(In reply to Dave Hunt (:davehunt) from comment #35) 
> Does the timeout need to be a constant if it's only used once in the test?

This is in line with our coding style in functional tests so I'm fine with it.

(In reply to Henrik Skupin (:whimboo) from comment #36)
> If we load the SWF file directly we probably don't have to wait for the 
> plugin content to be loaded.

Agreed.

r+
(Reporter)

Comment 38

7 years ago
Comment on attachment 563414 [details] [diff] [review]
patch v1.2 all branches [backed-out]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/c0bbed49effe (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/4fd1589a4b61 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/7c4b46fba081 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/80bd23436d69 (mozilla-release)
Attachment #563414 - Attachment description: patch v1.2 all branches → patch v1.2 all branches [checked-in]
(Reporter)

Comment 39

7 years ago
Please mark verified if it passes tomorrow's testrun, reopen otherwise.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
It seems that 50s is not quite enough as a timeout for page loading 

http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/f1eef9636e64aea710c3d8b5d73f562a

The test is currently failing with 

'controller.waitForPageLoad(): Timeout waiting for page loaded.'

Reopening and will file a failing bug. 
I guess we need to wait more for the page to load.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 41

7 years ago
No need to open a separate bug for the failure; simply take care of it on this bug. Thanks
Depends on: 691093
(Reporter)

Comment 42

7 years ago
Comment on attachment 563414 [details] [diff] [review]
patch v1.2 all branches [backed-out]

Backed-out:
http://hg.mozilla.org/qa/mozmill-tests/rev/8a3c01483a81 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/cb1008e04413 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/b264c6099b48 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/f27de6a70bf2 (mozilla-release)
Attachment #563414 - Attachment description: patch v1.2 all branches [checked-in] → patch v1.2 all branches [backed-out]
(Reporter)

Comment 43

7 years ago
Test has been backed out across all branches. Since the larger timeout has failed, I think we will have to source a shorter video. Would a 10 second snippet video be sufficient for testing purposes?
 (In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #43)
> Test has been backed out across all branches. Since the larger timeout has
> failed, I think we will have to source a shorter video. Would a 10 second
> snippet video be sufficient for testing purposes?

I think getting a new sample video will block this test for a long time - what can I do to make things go a little bit faster? 
Are there some specific requirements for videos which can guide me in finding one?
(Reporter)

Comment 45

7 years ago
Well, what I'm thinking is that we take the videos we already have but edit them down to 10s snippets.
(Reporter)

Comment 46

7 years ago
FYI, if this test is blocked on getting a shorter video, I'm fine with that. Hacking in a longer timeout is far less reliable.
(Reporter)

Comment 48

7 years ago
I've reached out to Rainer, our resident video expert, to see what can be done.
(Reporter)

Comment 49

7 years ago
I now have 2 snippet videos thanks to Rainer and Spencer:

SWF is 729 KB vs 4,505 KB
FLV is 681 KB vs 5,120 KB

I will try to get those uploaded to mozqa.com today. Question is how do we want these named? I'd really like to keep the original SWF and FLV (not overwriting them). They could be useful in the future and it would be a shame to need them and have to re-source them.

My suggestion is to call these new files:
 * sample-swf-video-snip.swf
 * sample-flv-video-snip.flv

Any objections?
What are the durations of the videos? Perhaps we could name them after that? For example: sample-swf-video-5s.swf
(Reporter)

Comment 51

7 years ago
(In reply to Dave Hunt (:davehunt) from comment #50)
> What are the durations of the videos? Perhaps we could name them after that?
> For example: sample-swf-video-5s.swf

I believe they are 10s each -- so sample-swf-video-10s.swf?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #51)
> (In reply to Dave Hunt (:davehunt) from comment #50)
> > What are the durations of the videos? Perhaps we could name them after that?
> > For example: sample-swf-video-5s.swf
> 
> I believe they are 10s each -- so sample-swf-video-10s.swf?

Can we go with Anthony's suggestion to de-block this?
(Reporter)

Comment 54

7 years ago
Created attachment 574393 [details]
Minified FLV
(Reporter)

Comment 55

7 years ago
Created attachment 574394 [details]
Minified SWF
(Reporter)

Comment 56

7 years ago
Created attachment 574401 [details] [diff] [review]
Patch v2.0: minified flash videos [checked-in]

This patch adds the two attached minified flash videos to the litmus-data repository. Once checked in, it should be cloned to mozqa.com on a cron-job.
Attachment #574401 - Flags: review?(dave.hunt)
Attachment #574401 - Flags: review?(dave.hunt) → review+
(Reporter)

Comment 57

7 years ago
Comment on attachment 574401 [details] [diff] [review]
Patch v2.0: minified flash videos [checked-in]

Landed:
http://hg.mozilla.org/qa/litmus-data/rev/b4069ec7874e
Attachment #574401 - Attachment description: Patch v2.0: minified flash videos → Patch v2.0: minified flash videos [checked-in]
Created attachment 576477 [details] [diff] [review]
patch v2 all branches

Fixed to use the new minified video. Hopefully will not fail
Attachment #576477 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 59

7 years ago
Comment on attachment 576477 [details] [diff] [review]
patch v2 all branches

This test still points to the old file. You should be using sample-flv-video-10s.swf
Attachment #576477 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Comment 60

7 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #59)
> This test still points to the old file. You should be using
> sample-flv-video-10s.swf

sample-swf-video-10s.swf that is.
Created attachment 576711 [details] [diff] [review]
patch v3 all [backed-out]

Again, problems with refreshing patches ... 
Sorry
Attachment #576477 - Attachment is obsolete: true
Attachment #576711 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 62

7 years ago
Comment on attachment 576711 [details] [diff] [review]
patch v3 all [backed-out]

Patch looks fine -- please remind me what is the command you were using to test this patch so I can confirm?

Thanks
Attachment #576711 - Flags: review?(anthony.s.hughes) → review+
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #62)
> Comment on attachment 576711 [details] [diff] [review] [diff] [details] [review]
> patch v3 all branches
> 
> Patch looks fine -- please remind me what is the command you were using to
> test this patch so I can confirm?
> 
> Thanks

You need the automation repo cloned. that is /automation/mozmill-automation then use the testrun_endurance.py 

The specific command is 

python testrun_endurance.py --report=URL --repository=PATH TO LOCAL REPO --iterations=IT_NUMBER --entities=ENTITIES_NUMBER PATH TO FIREFOX BUILD 

* Please be careful to be in the same branch on the mozmill-tests/ repo as the firefox build 
you are using
* --report=URL ; the URL must be http://mozmill-crowd.brasstacks.mozilla.com/db/
(Reporter)

Comment 64

7 years ago
Comment on attachment 576711 [details] [diff] [review]
patch v3 all [backed-out]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/895b5b7b697b (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/b0ce65162049 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/2918fd5bf296 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/ca1d3214e853 (mozilla-release)
Attachment #576711 - Attachment description: patch v3 all branches → patch v3 all branches [checked-in]
(Reporter)

Comment 65

7 years ago
Please verify with tomorrow's testrun.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Unfortunately we can't tell how far through the test this was - it may be something that deteriorates over time. Have we tried running with 100 iterations and 10 entities locally? Perhaps we need to revisit increasing the timeout further for this test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

7 years ago
Duplicate of this bug: 707656
(Reporter)

Comment 69

7 years ago
Comment on attachment 576711 [details] [diff] [review]
patch v3 all [backed-out]

Backed-out:
http://hg.mozilla.org/qa/mozmill-tests/rev/4157e88f70d2 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/ae714fd9b496 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/9605c0679be6 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/299f9768e804 (mozilla-release)
Attachment #576711 - Attachment description: patch v3 all branches [checked-in] → patch v3 all [backed-out]
(Reporter)

Comment 70

7 years ago
I'm now running the test proposed in comment 67 on qa-set.
(Reporter)

Comment 71

7 years ago
The throbber appears to hang for ~15s while the test page loads. Opening the activity monitor shows that Nightly gets into a "not responding" state once in a while which it does recover from.

I'm wondering if what we are looking at here is not an issue with the test but a performance / GC regression in Firefox itself?
(Reporter)

Updated

7 years ago
Blocks: 671479
(Reporter)

Comment 72

7 years ago
After nearly 30 minutes on the same test, time to load the tab has increased from ~15s to ~30s. I'm going to see if this is reproducible manually.
(Reporter)

Comment 73

7 years ago
Definitely not reproducible manually though it seems to be an obvious performance degradation when run with our Mozmill environment.
(Reporter)

Updated

7 years ago
Depends on: 708270
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #73)
> Definitely not reproducible manually though it seems to be an obvious
> performance degradation when run with our Mozmill environment.

Should we increase the timeout again, as comment 67 suggests? How would we want the work here to continue?
(Reporter)

Comment 75

7 years ago
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #74)
> Should we increase the timeout again, as comment 67 suggests? How would we
> want the work here to continue?

We need to investigate further with bug 708270 before making that decision. Anything you can do to help would be great.
Dave, is that a test which would still be useful for us?
Assignee: vlad.mozbugs → nobody
Status: REOPENED → NEW
In that case please re-request review for the patch so we can get it re-landed. In this case however we would at least need to update the license block in the patch to MPL2.
(Assignee)

Comment 80

6 years ago
Created attachment 739082 [details] [diff] [review]
patch v3.1

I have modified the patch to have the new license and also made changes:
- added aModule parameter to setupModule
- added tearDown because we might be failing at waitForPageLoad and not close the tabs before the end of the test
- added manifest.ini inside the new folder
- added the new manifest.ini to the endurance/manifest.ini

Reports were done together with the new changes from bug 671479 since there were issues running both of them in a single testrun.
Linux: http://mozmill-crowd.blargon7.com/#/endurance/report/8ec48e7ab0431a61b624e36d31ff4d2c
Windows: http://mozmill-crowd.blargon7.com/#/endurance/report/c8094a822ef568b588c5eddaca00cf92
MAC: http://mozmill-crowd.blargon7.com/#/endurance/report/c8094a822ef568b588c5eddaca0fe07c

NOTE: It takes about 2 hours for the endurance runs after adding these two tests
Attachment #576711 - Attachment is obsolete: true
Attachment #739082 - Flags: review?(andreea.matei)
(Assignee)

Updated

6 years ago
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Comment on attachment 739082 [details] [diff] [review]
patch v3.1

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

Please use the video with no sound. Otherwise, looks good to me.

::: tests/endurance/testFlash_SWFVideoURL/test1.js
@@ +7,5 @@
> +var endurance = require("../../../lib/endurance");
> +var tabs = require("../../../lib/tabs");
> +
> +const TEST_DOMAIN = "http://www.mozqa.com/";
> +const TEST_PAGE = TEST_DOMAIN + "data/firefox/plugins/flash/sample-swf-video-10s.swf";

Please use sample-swf-video-10s-nosound.swf
Attachment #739082 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 82

6 years ago
Created attachment 739504 [details] [diff] [review]
patch v3.2

Modified to use the correct video file and also changed to BASE_URL and TEST_URL const due to bug 848649.

The reports for Mac and Windows are below:
http://mozmill-crowd.blargon7.com/#/endurance/report/c8094a822ef568b588c5eddaca249b2f
http://mozmill-crowd.blargon7.com/#/endurance/report/c8094a822ef568b588c5eddaca24a0e2

NOTE: Since we are not landing new tests today, I will provide the reports for Linux on Monday.
Attachment #739082 - Attachment is obsolete: true
Attachment #739504 - Flags: review?(andreea.matei)
Comment on attachment 739504 [details] [diff] [review]
patch v3.2

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

::: tests/endurance/testFlash_SWFVideoURL/test1.js
@@ +7,5 @@
> +var endurance = require("../../../lib/endurance");
> +var tabs = require("../../../lib/tabs");
> +
> +const BASE_URL = "http://www.mozqa.com/";
> +const TEST_URL = BASE_URL + "data/firefox/plugins/flash/sample-swf-video-10s-nosound.swf";

We can remove BASE_URL and use the seconds with the full link.

@@ +25,5 @@
> +    if (aAddon.isActive && aAddon.type === "plugin" && aAddon.name === "Shockwave Flash")
> +      return true;
> +  });
> +
> +  if (isFlashActive[0] !== true) {

!isFlashActive[0] would work also.

@@ +26,5 @@
> +      return true;
> +  });
> +
> +  if (isFlashActive[0] !== true) {
> +    testFlashURL.__force_skip__ = "No enabled Flash plugin detected";

Please also skip teardownModule()

@@ +51,5 @@
> +      controller.open(TEST_URL);
> +      controller.waitForPageLoad(TIMEOUT_PAGE);
> +      enduranceManager.addCheckpoint("Web page has been loaded");
> +    });
> +    // Close all tabs

Remove the comment as it is clear what the method does and leave a blank space to separate the blocks.
Attachment #739504 - Flags: review?(andreea.matei) → review-
Comment on attachment 740331 [details] [diff] [review]
patch v3.3

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

Looks good, lets see how it works and how it's affecting the memory.
The testrun Daniela provided took almost 2 hours (120 minutes) and the timeout in Jenkins is set to 300 minutes, so we're safe.

http://hg.mozilla.org/qa/mozmill-tests/rev/f295a9e2cbee (default)
Attachment #740331 - Flags: review?(andreea.matei) → review+
status-firefox23: --- → fixed
Comment on attachment 742956 [details] [diff] [review]
patch v1.0 for Release

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/09656209429c (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/10cd977017ac (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/3daa9fa0125a (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/034b8d11bd0b (esr17)

Thanks Daniela, another endurance test in house :)
Attachment #742956 - Flags: review?(andreea.matei) → review+
Attachment #742989 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago6 years ago
status-firefox20: --- → fixed
status-firefox21: --- → fixed
status-firefox22: --- → fixed
status-firefox-esr17: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.