-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Test discovery using Python pytest #4795
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4795 +/- ##
========================================
Coverage ? 62%
========================================
Files ? 375
Lines ? 14714
Branches ? 1159
========================================
Hits ? 9037
Misses ? 5476
Partials ? 201 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. There are a few odds and ends to clean up. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a few things that I'd like addressed. Otherwise, LGTM.
FWIW, earlier reviews were incomplete; I stopped reviewing once I reached a critical mass of review comments. With this review I made it all the way through. :)
cwd: options.cwd, | ||
throwOnStdErr: true | ||
}; | ||
return execService.exec([DISCOVERY_FILE, ...options.args], spawnOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adapter script has a certain set of args that are required and will be the same every time. I'd expect those to be generated as close to this exec()
call as possible. Why are we leaving those args up to some other class?
I imagine the code here looking something like this:
const args = [DISCOVERY_FILE, 'discover', options.tool, '--', ...options.toolargs]
return execService.exec(args, spawnOptions);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we leaving those args up to some other class?
Separation of concerns... creating args is one thing, and executing it another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not buying this. What are the concerns that need separating? The args that I'm talking about are intrinsic to the adapter script. They're fundamentally coupled. The corresponding code should be together. Other, tool-specific args certainly deserve separation. However, I'm not talking about those. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm keeping them separate as it's easier to manage.
Arguments need to get build/parsed, sepearately for PyTest, unit test and nose, and having them done in three separate places makes sense rather than having case
statements in one file.
I guess we're going to have to agree to disagree, however lets discuss this in engineering meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sticking with this. Aside from a few open comments in my last review, LGTM. I'll assume you'll sort those out. :)
@ericsnowcurrently I've added two separate tests for the items you've suggested. |
I'm still not getting pytest discovered from my Miniconda installation. I installed it with both conda and pip and either way, the extension doesn't discover it. I have it installed at the default path of C:\ProgramData\Miniconda3. |
@WSLUser Please file an issue so we can help you. |
For #4035.
This is a refactor of #4695.
package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed)