Closed Bug 1586297 Opened 1 year ago Closed 1 year ago

debugger jest test are failing (silently on TRY)

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 unaffected, firefox67 unaffected, firefox68 unaffected, firefox69 unaffected, firefox70 unaffected, firefox71 fixed)

RESOLVED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Steps to reproduce

  1. Run yarn test within the debugger folder

Expected results

Tests pass

Actual results

Tests fail:

FAIL   test  src/actions/sources/tests/loadSource.spec.js
  ● Test suite failed to run

    Cannot find module 'devtools/client/shared/thread-utils' from 'targets.js'

      11 | 
      12 | // $FlowIgnore
    > 13 | const { defaultThreadOptions } = require("devtools/client/shared/thread-utils");
         |                                  ^
      14 | 
      15 | type Args = {
      16 |   currentTarget: Target,

      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:221:17)
      at Object.require (src/client/firefox/targets.js:13:34)

This wasn't picked up by treeherder because in the runner we check for the numFailedTests property of the result JSON. In our case, this property is still at 0, but others are showing there's an issue.

We should do like the devtools runner, and check the length of the errors array we build in devtools/client/debugger/bin/try-runner.js#78-88

The following patch makes TRY fail, as it should:

diff --git a/devtools/client/debugger/bin/try-runner.js b/devtools/client/debugger/bin/try-r
--- a/devtools/client/debugger/bin/try-runner.js
+++ b/devtools/client/debugger/bin/try-runner.js
@@ -75,8 +75,6 @@ function jest() {
   const jsonOut = out.substring(out.indexOf("{"), out.lastIndexOf("}") + 1);
   const results = JSON.parse(jsonOut);

-  const failed = results.numFailedTests == 0;
-
   // The individual failing tests are in jammed into the same message string :/
   const errors = [].concat(
     ...results.testResults.map(r =>
@@ -85,7 +83,7 @@ function jest() {
   );

   logErrors("jest", errors);
-  return failed;
+  return errors.length == 0;
 }

 function stylelint() {

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce74de893c46bd9d12f8dfad85be4192fcd31ccd&selectedJob=269787410

Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED

Some tests were failing because we require an absolute
path from the debugger source. We add a new entry in jest
config's moduleNameMapper to reroute all the absolute require
of devtools modules to the right path.
To fix the failures we also need to add a fixture for Services
as it's loaded in one of the fail we use in the debugger.

We take this opportunity to fix debugger's try runner.
Currently, we were only checking the numFailedTests property
of the resulting JSON when running tests.
But in some case, this property would still be 0, but other
properties would indicate the some tests fail.
In order to not have to go through all those properties, we
simply check that the array of error messages we build is
empty to assert if the tests were successfully ran.

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c13a124f1515
Fix debugger's jest failures. r=jlast.
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
You need to log in before you can comment on or make changes to this bug.