937740 - Enable jsdownloads unit tests on B2G

Status: RESOLVED INVALID

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

We don't run most of jsdownloads tests for now on b2g, but support is added in bug 936342 and bug 934677.

Paolo, can you point me to the disabled tests?

Comment 1

[:Paolo Amadini]

(In reply to Fabrice Desré [:fabrice] from comment #0)
> Paolo, can you point me to the disabled tests?

The tests are all contained in the "toolkit/components/jsdownloads/test/unit" folder.

You can see those in the Desktop build log:

https://tbpl.mozilla.org/php/getParsedLog.php?id=30476271&tree=Mozilla-Central&full=1

Those are not run on B2G for the same revision:

https://tbpl.mozilla.org/php/getParsedLog.php?id=30477293&tree=Mozilla-Central&full=1

I've noticed that the only xpcshell run visible in TBPL is for the "B2G ICS Emulator Opt" configuration.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Taking.

Currently some tests depends on places (PlacesTestUtils actually), so those tests should be skipped on b2g because places is not built in b2g.

We have no easy way to skip test on a certain platform.
I've opened bug 1150818 and bug 1150822 to skip test easily.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP v1

* Tests for DownloadObserver in test_DownloadIntegration.js split into a new file (test_DownloadObserver.js)
* Rest of tests in test_DownloadIntegration.js skip since those tests are related to DownloadIntegration.getSystemDownloadsDirectory. (i.e. browser window is needed)
* test_DownloadLegacy.js skips since legacy saver is unnecessary on b2g I think
* tests using places utility in test_DownloadList.js skip 
* tests related to Downloads.getSystemDownloadsDirectory in test_Downloads skip

With this patch rest of tests almost work fine except a failure in test_DownloadCore.js:

 4:27.05 LOG: Thread-1 ERROR The download should have failed.
    test_DownloadCore.js -> file:///data/local/tests/xpcshell/toolkit/components/jsdownloads/test/unit/common_test_Download.js:test_error_target:1281
    self-hosted:next:675
    _run_next_test@/data/local/tests/xpcshell/head.js:1416:9
    do_execute_soon/<.run@/data/local/tests/xpcshell/head.js:653:9
    _do_main@/data/local/tests/xpcshell/head.js:207:5
    _execute_test@/data/local/tests/xpcshell/head.js:513:5
    @-e:1:1

I have no idea why the test failed now. Need to investigate.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

The failure of test_error_target might be affected by other tests.

Moving test_error_target and test_error_restart at the head of common_test_Download.js solves the failure.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> The failure of test_error_target might be affected by other tests.
> 
> Moving test_error_target and test_error_restart at the head of
> common_test_Download.js solves the failure.

whoa, moving those tests did not solve the failure. It makes those tests intermittent failure...

Comment 6

[Hiroyuki Ikezoe (:hiro)]

From test_error_target in common_test_Download.js:

        download = yield Downloads.createDownload({
          source: httpUrl("source.txt"),
          target: targetFile,
        });
        yield download.start();
      } else {
        // When testing DownloadLegacySaver, we cannot be sure whether we are
        // testing the promise returned by the "start" method or we are testing
        // the "error" property checked by promiseDownloadStopped.  This happens
        // because we don't have control over when the download is started.
        download = yield promiseStartLegacyDownload(null,
                                                    { targetFile: targetFile });
        yield promiseDownloadStopped(download);
      }
      do_throw("The download should have failed.");

Though I don't investigate implementation of DownloadCore.jsm, before throwing ex exception we should wait for start of saving the target file on local storage?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Clearing needinfo request.

The target file permission was changed to 666 in failure case.

Comment 8

[:Paolo Amadini]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> * Tests for DownloadObserver in test_DownloadIntegration.js split into a new
> file (test_DownloadObserver.js)

Nice!

> * Rest of tests in test_DownloadIntegration.js skip since those tests are
> related to DownloadIntegration.getSystemDownloadsDirectory. (i.e. browser
> window is needed)

Okay, maybe B2G needs a bug on file to implement this test in the appropriate framework, if one isn't there already.

> * test_DownloadLegacy.js skips since legacy saver is unnecessary on b2g I
> think

It's actually used and an important piece, it's how single-click downloads (for example pages with Content-Disposition headers) are handled. Did you observe specific failures with them?

> * tests using places utility in test_DownloadList.js skip 
> * tests related to Downloads.getSystemDownloadsDirectory in test_Downloads
> skip

Okay.

Comment 9

[:Paolo Amadini]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> The target file permission was changed to 666 in failure case.

Can you elaborate?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to :Paolo Amadini from comment #8)

> > * Rest of tests in test_DownloadIntegration.js skip since those tests are
> > related to DownloadIntegration.getSystemDownloadsDirectory. (i.e. browser
> > window is needed)
> 
> Okay, maybe B2G needs a bug on file to implement this test in the
> appropriate framework, if one isn't there already.

I will open a bug for it.

> > * test_DownloadLegacy.js skips since legacy saver is unnecessary on b2g I
> > think
> 
> It's actually used and an important piece, it's how single-click downloads
> (for example pages with Content-Disposition headers) are handled. Did you
> observe specific failures with them?

Thanks! I did not know the fact. Now I tried, and there is the same failure in test_DownloadCore.js caused by OS.file.open bug (bug 1154955). So the failure will be solved by that bug.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

The failures of test_error_target and test_error_restart hid other intermittent failures.
test_public_and_private and others sometimes fails on local.

Comment 12

[:Paolo Amadini]

Closing this old B2G bug.