Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix tests in Debugging/DebugServiceTests.cs and simplify faulty script path logic #1540

Merged
merged 8 commits into from
Aug 12, 2021
Merged

Fix tests in Debugging/DebugServiceTests.cs and simplify faulty script path logic #1540

merged 8 commits into from
Aug 12, 2021

Conversation

andyleejordan
Copy link
Member

@andyleejordan andyleejordan commented Aug 11, 2021 •
edited by rjmholt
Loading

This resolves #1442, and sets me up to add a test around debugging code that uses an object from System.Windows.Forms.

This PR also:

  • Fixes some NullReferenceExceptions exposed by the restored tests
  • Removes some broken script path logic
  • Replaces some older hard-coded empty array logic with the newer .NET Standard API
  • Fixes a deadlock that occurs when debugging is aborted

@andyleejordan andyleejordan requested a review from rjmholt August 11, 2021 20:35
@ghost ghost added the Area-Test label Aug 11, 2021
@andyleejordan andyleejordan added the Issue-Bug A bug to squash. label Aug 11, 2021
rjmholt
rjmholt reviewed Aug 11, 2021
View reviewed changes
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The restored test code looks good, but there seem to be some product changes that aren't explained or mentioned in the title or description. I've read through the commit descriptions, but I think it's worth adding an explanation of the changes and how they relate to the testing in the PR description for posterity.

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs Outdated Show resolved Hide resolved
@andyleejordan
Copy link
Member Author

andyleejordan commented Aug 11, 2021 •
edited
Loading

Dang, these passed locally. I spoke too soon, there was more to fix. All fixed now.

@andyleejordan andyleejordan force-pushed the andschwa/debugger-tests branch from f2f2d46 to 4470f49 Compare August 11, 2021 23:36
@andyleejordan
Ignore IDE rule to drop unneeded .this prefix
19c44f7
@andyleejordan
Uncomment DebugServiceTests.cs
7c41c5b
@andyleejordan
Validate hashtable key is not null in the debug service
66ba44e 
The `scriptPath` (which amusingly could also be a script block) is used
as a key in a hashtable of breakpoints in the debug service. To avoid a
(that specifically showed up in the unit tests), we validate that a
non-empty (and non-null) value is provided.
@andyleejordan
Avoid mangling paths given to ExecuteScriptWithArgsAsync
da43937 
Some extant code was using PowerShell to get the current working
directory and then was hardcoded to prepend that to the given path, in
order to make an "absolute path." This was buggy, so we fixed it by only
prefixing non-rooted paths with the CWD, and do so with `Path.Combine`.
@andyleejordan
Avoid null-dereference of _languageServer in OnDebuggerStop handler
b6571a5 
This primarily occured during unit testing, where the language server is
not instantiated. Note that elsewhere in this file it is treated
similarly.
@andyleejordan
Use Array.Empty instead of new []
5d5d7bc
@andyleejordan
Fix DebugService.Abort() deadlock
301ee69 
With the default value of `true` for `shouldWaitForExit` this would
deadlock because it exepcted the pipeline execution thread to clean it
up...except it was waiting in the pipeline execution thread.
@andyleejordan andyleejordan force-pushed the andschwa/debugger-tests branch from 4470f49 to 10acadc Compare August 12, 2021 01:08
@andyleejordan
Copy link
Member Author

The restored test code looks good, but there seem to be some product changes that aren't explained or mentioned in the title or description. I've read through the commit descriptions, but I think it's worth adding an explanation of the changes and how they relate to the testing in the PR description for posterity.

The only posterity that exists is Git commits. PRs disappear. We might not be on GitHub in five years (we weren't hardly more than five years ago!)

@rjmholt
Copy link
Contributor

rjmholt commented Aug 12, 2021

The only posterity that exists is Git commits. PRs disappear. We might not be on GitHub in five years (we weren't hardly more than five years ago!)

Ok, well seeing as how it's just a copy and paste and others on the team tend to look at the PRs, I'll do it for you

@rjmholt rjmholt changed the title Fix tests in Debugging/DebugServiceTests.cs Fix tests in Debugging/DebugServiceTests.cs and simplify faulty script path logic Aug 12, 2021
@rjmholt rjmholt self-requested a review August 12, 2021 01:15
@rjmholt
Copy link
Contributor

rjmholt commented Aug 12, 2021

@andschwa looks like you're still pushing commits, let me know when you want a review

@andyleejordan
Copy link
Member Author

andyleejordan commented Aug 12, 2021 •
edited
Loading

Dah they DO literally all pass locally now. Ugh checking CI. Apparently differences between Run Test and Invoke-Build Test. Oof.

@andyleejordan
Fix all DebugServiceTests
648cd3e 
A number of items needed to be fixed:

* `Path.Combine` instead of `Path.Join` because of .NET changes
* `ConfigureAwait(false)` because of our rule to always include that
* Pass actual script path to `BreakpointDetails.Create` so the debug
  service has a valid value for its hashtable key
* Call `Close()` instead of `Dispose()` (which changed) on
  `PowerShellContextService`
* Make assertion in `AssertDebuggerStopped` case-insensitive
* Increase cancellation token source timeouts to 10 seconds
* Avoid waiting for PowerShell context state change that never happens
  and isn't required
* Delete unused fields or local variables
* Avoid issue with breakpoint ordering (see comment)
* Consistently abort the debugger and wait for the `executeTask`
* Make `Get-Process` test cross-platform (and execute the right line)
@andyleejordan andyleejordan force-pushed the andschwa/debugger-tests branch from 10acadc to 648cd3e Compare August 12, 2021 02:18
@andyleejordan
Copy link
Member Author

Yay, I did it, they're all working now in CI too. @rjmholt Could you kindly review, with specific attention to 648cd3e?

@andyleejordan
Copy link
Member Author

Also we just went from 1,543 in CI to 1,829 🥳

@andyleejordan andyleejordan merged commit 879857f into master Aug 12, 2021
@andyleejordan andyleejordan deleted the andschwa/debugger-tests branch August 12, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmholt rjmholt rjmholt approved these changes

Assignees
No one assigned
Labels
Area-Test Issue-Bug A bug to squash.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix tests in Debugging/DebugServiceTests.cs
2 participants
@andyleejordan @rjmholt