Closed Bug 1028090 Opened 6 years ago Closed 5 years ago

--debugger argument does not support Visual Studio (Windows debugging)

Categories

(Testing :: XPCShell Harness, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

Attachments

(1 file, 1 obsolete file)

The xpcshell-test suite does not seem to support debugging of script on Windows. 
It fails with the error:

$ ./mach xpcshell-test --debugger WDExpress.exe --interactive --debugger-interactive my_test.js
Error: Path WDExpress.exe doesn't exist.
Attached patch bug_xpcshell_dbg.patch (obsolete) — Splinter Review
This simple patch adds the support for Visual Studio and Visual C++ Express.

The following command correctly spawns a Visual Studio instance and attaches the debugger:

./mach xpcshell-test --debugger WDExpress.exe --interactive --debugger-interactive my_test.js
Assignee: nobody → alessio.placitelli
Attachment #8443367 - Flags: review?(jmaher)
Comment on attachment 8443367 [details] [diff] [review]
bug_xpcshell_dbg.patch

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

this patch seems too easy, I don't have an easy way to test this- but looking at the debugger hooks we seem to be setting the right things.

THanks for supporting our #1 os of users.
Attachment #8443367 - Flags: review?(jmaher) → review+
You're welcome! 

You're right, it looks like it's extremely simple. I wonder if there's anything else I can do to or somebody else to ask a feedback to. Suggestions?

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 8443367 [details] [diff] [review]
> bug_xpcshell_dbg.patch
> 
> Review of attachment 8443367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this patch seems too easy, I don't have an easy way to test this- but
> looking at the debugger hooks we seem to be setting the right things.
> 
> THanks for supporting our #1 os of users.
Flags: needinfo?(jmaher)
If this works for you locally, then that is the testing we need.  Further work to complete this loop would be to make a comment in the MDN article:
https://developer.mozilla.org/en-US/docs/Mochitest#Debugging_individual_tests

maybe just indicate which debuggers we support.
Flags: needinfo?(jmaher)
bug 928397 claims what we want is "devenv -debugexe ". Also I wish "mach debug" and "--debugger in our test harnesses" shared code.
Sure, that's true: that's why I've added both devenv support (Visual Studio - non express) and WDExpress (Visual C++ Express Edition) - See http://benjamin.smedbergs.us/blog/2014-03-10/use-debugexe-to-debug-apps-in-visual-studio/ .

I'll take a shot at "mach debug" as well! :) What do you mean by "--debugger in our test harnesses" ?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> bug 928397 claims what we want is "devenv -debugexe ". Also I wish "mach
> debug" and "--debugger in our test harnesses" shared code.
Oops, sorry, hadn't read your patch. Cool! You fixed the "--debugger for test harness" case. What I was saying is that I wish that was the same code behind "mach debug" so we wouldn't have to fix this in two places!
Ah, awesome :)

I'll dig into that and take on Bug 928397. Meanwhile, should I push this to m-c?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Oops, sorry, hadn't read your patch. Cool! You fixed the "--debugger for
> test harness" case. What I was saying is that I wish that was the same code
> behind "mach debug" so we wouldn't have to fix this in two places!
Yes, that's fine. Thanks for the patch!
I'm assuming this is ready to go?
Keywords: checkin-needed
Yes, but bug 928397 embeds the code of this patch and moves it to a new package (mozdebug), so I'm not sure it makes sense to commit this one. I was waiting on your feedback on that bug before adding the checkin flag! Should we commit it anyway?
Flags: needinfo?(ted)
Yeah, it's fine, we can land this one until I get the other one reviewed.
Flags: needinfo?(ted)
patch didn't apply :

applying bug_xpcshell_dbg.patch
patching file build/automationutils.py
Hunk #1 FAILED at 67
1 out of 1 hunks FAILED -- saving rejects to file build/automationutils.py.rej
patch failed, unable to continue (try -v)

could you take a look at this and maybe rebase against  a current tree ?

also we need maybe here a try run or ?
Keywords: checkin-needed
Here we go! I've rebased the patch to work with the latest m-c.

I'm not quite sure a try push would be useful, since I'm not sure the part which I've modified gets tested by that. Does it?
Attachment #8443367 - Attachment is obsolete: true
Flags: needinfo?(ted)
A try run wouldn't be useful here, no. This is almost DONTBUILD (although it does touch automationutils.py, so not completely).
Flags: needinfo?(ted)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8923901b8b54
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.