1441310 - Refactor Breakpoint tests

Status: RESOLVED FIXED

(DevTools :: Debugger, task, P3)

Description

[Jason Laster [:jlast]]

We recently refactored the stepping tests to use async/await, which make the tests much easier to read (1439711). We should do the same thing with the breakpoint unit tests in server/tests/unit.

https://searchfox.org/mozilla-central/search?q=breakpoint-0&case=false&regexp=false&path=

Comment 1

[Jason Laster [:jlast]]

An ambitious answer would be to explore an automated refactor
https://searchfox.org/mozilla-central/search?q=addOneTimeListener(%22paused%22&case=false&regexp=false&path=

Comment 2

[malikanshul29]

Attachment: patch

Tests 1-8 are refactored in this patch

Comment 3

[Jason Laster [:jlast]]

Comment on attachment 8955392 [details] [diff] [review]
patch

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

Looks great! A couple of small nits, but otherwise really good.

On a personal note, sorry for the delay. It took me awhile to figure out how to get the patch.

::: devtools/server/tests/unit/test_breakpoint-01.js
@@ -26,2 @@
>    gClient = new DebuggerClient(server.connectPipe());
> -  gClient.connect().then(function () {

something broke for this file's patch when i applied it. not sure what though

@@ +58,5 @@
> +      `                                 // 1
> +      var line0 = Error().lineNumber;   // 2
> +      debugger;                         // 3
> +      var a = 1;                        // 4
> +      var b = 2;`                       // 5,

i think this comma should be next to the `

@@ -66,5 @@
> -  });
> -
> -  /* eslint-disable */
> -  Cu.evalInSandbox(
> -    "var line0 = Error().lineNumber;\n" +

we can take this out. I think we can also have the first line be at the `

When we make these changes, the line numbers should no longer be + 2. 

I think i did this for the stepping refactor

::: devtools/server/tests/unit/test_breakpoint-02.js
@@ -49,5 @@
> -    let source = gThreadClient.source(packet.frame.where.source);
> -    source.setBreakpoint(location, function (response) {
> -      // Eval scripts don't stick around long enough for the breakpoint to be set,
> -      // so just make sure we got the expected response from the actor.
> -      Assert.notEqual(response.error, "noScript");

did this fail in the new version? if so, did you try `const response = await source.setBreakpoint(location)` if you have that you can likely do the same thing...

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8955392 [details] [diff] [review]
patch

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

Thank you for the patch.  It looks good to me overall.

Jason asked me to take a look at this.  I agree with Jason's comments, and I found a couple of minor nits (which I think also appear in other spots in the patch as well, not noted by me.  These aren't serious though.

::: devtools/server/tests/unit/test_breakpoint-08.js
@@ +46,5 @@
>  
> +  const [bpResponse, bpClient] = await source.setBreakpoint(location);
> +  // Check that the breakpoint has properly skipped forward one line.
> +  Assert.equal(bpResponse.actualLocation.source.actor, source.actor);
> +  Assert.equal(bpResponse.actualLocation.line, location.line + 1);

Most of the other spots were changed to "equal", removing the "Assert".

@@ +70,4 @@
>  
> +  function evaluateTestCode() {
> +    /* eslint-disable */
> +    Cu.evalInSandbox("var line0 = Error().lineNumber;\n" +

Seems like this should use a template string.

Comment 5

[Jason Laster [:jlast]]

Comment on attachment 8955392 [details] [diff] [review]
patch

needs some tweaks, but it's getting close

Comment 6

[Paarmita Bhargava]

Hey,
I would like to work on this issue?

Comment 7

[Jason Laster [:jlast]]

Great! Would you like to start off with anshul's patch? I'm sure a lot has changed so it would be nice to see how much is still relevant.

Comment 8

[Paarmita Bhargava]

Can you please let me know what needs to be done after Anshul's patch?

Comment 9

[Jason Laster [:jlast]]

HI Paarmita, I just re-read the tests and they are much better than a year ago.

I'd ignore anshul's patch and instead read through the breakpoint tests and look for callbacks and try to replace them with promises.

For example addOneTimeListener can often be replaced with await waitForEvent

Note: the helpers are in test-head.js in that directory

Comment 10

[Paarmita Bhargava]

Thanks for the help. I am working on it.

Comment 11

[Arun Kumar Mohan]

Hey Jason, I would like to take a stab at this issue. Do you mind assigning it to me?

Comment 12

[Arun Kumar Mohan]

Hey Jason,

I am having issues with running the tests. I tried running ./mach test devtools/server/tests/unit/test_breakpoint-06.js and got the following output:

Error running mach:

    ['test', 'devtools/server/tests/unit/test_breakpoint-06.js']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You can invoke |./mach busted| to check if this issue is already on file. If it
isn't, please use |./mach busted file| to report it. If |./mach busted| is
misbehaving, you can also inspect the dependencies of bug 1543241.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

BuildEnvironmentNotFoundException: config.status not available. Run configure.

I then tried running ./mach configure and ran into the following error:

ERROR: Could not find installed Xcode Command Line Tools; run `xcode-select --install` and follow the instructions to install them then try again; if you wish to build without Xcode Command Line Tools installed, add the --disable-xcode-checks configure flag
 0:01.99 *** Fix above errors and then restart with               "./mach build"
 0:01.99 make: *** [configure] Error 1
mozilla-unified$ xcode-select --install
xcode-select: note: install requested for command line developer tools

This is weird because I have xcode installed on my machine. Not sure why mach is unable to find it.

Do you mind helping me get unstuck?

Comment 13

[Jason Laster [:jlast]]

Hi, I'm sorry this is not working for you. Could you stop by our slack and ask (https://devtools-html-slack.herokuapp.com/)?

Comment 14

[Arun Kumar Mohan]

Thanks Jason!

Comment 15

[Miriam]

Attachment: Refactor breakpoint server tests.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf5505380514
Refactor breakpoint server tests.

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/cf5505380514