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.

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

Make module context-aware #30

Merged
merged 9 commits into from
Apr 8, 2021
Merged

Make module context-aware #30

merged 9 commits into from
Apr 8, 2021

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Apr 5, 2021

Fixes microsoft/vscode#120570

  • Added tests
  • Set Appveyor to only use Node 12 and 14
  • Updated some dependencies (we're still using tslint, though)

@rzhao271 rzhao271 added the debt label Apr 5, 2021
@rzhao271 rzhao271 requested review from bpasero and deepak1556 April 5, 2021 18:26
@rzhao271 rzhao271 self-assigned this Apr 5, 2021
@rzhao271 rzhao271 changed the title Make context-aware, fixes microsoft/vscode#120570 Make module context-aware Apr 5, 2021
Copy link
Contributor

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Please add tests that uses the module simultaneously from main thread as well as nodejs worker threads

appveyor.yml Outdated
@@ -7,6 +7,7 @@ environment:
- nodejs_version: "10"
- nodejs_version: "11"
Copy link
Member

@Tyriar Tyriar Apr 6, 2021

Choose a reason for hiding this comment

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

Let's remove 10 and 11, they're pretty old at this point.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

👏 for the test

@deepak1556
Copy link
Contributor

@Tyriar appveyor is not running on this repo due to permission issues https://ci.appveyor.com/project/Tyriar/vscode-windows-process-tree/builds/38563417, can you reauthorize. Thanks!

@Tyriar
Copy link
Member

Tyriar commented Apr 6, 2021

Fixed it

@rzhao271
Copy link
Contributor Author

rzhao271 commented Apr 6, 2021

I'll go change the tests because the current worker tests don't call into native code.

@deepak1556
Copy link
Contributor

Additionally can you add a test that checks the memory usage of the process, use getProcessList after multiple workers have exited to ensure there is no memory leak.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Let's just keep node 12 and 14 in appveyor.yml

@rzhao271 rzhao271 requested review from deepak1556 and Tyriar April 6, 2021 18:53
@rzhao271
Copy link
Contributor Author

rzhao271 commented Apr 6, 2021

I didn't add the memory leak test because the delta between the initial memory and final memory amounts, even after enabling GC (by passing --enable-gc to mocha in package.json and by using global.gc() in the memory leak test), kept varying too much.

@rzhao271
Copy link
Contributor Author

rzhao271 commented Apr 6, 2021

Ready for review again

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks, very thorough 🙂

@rzhao271 rzhao271 merged commit 767e74e into main Apr 8, 2021
@deepak1556 deepak1556 deleted the rzhao271/context-aware branch April 8, 2021 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

windows-process-tree needs to be context-aware
4 participants