Update logic used to parse the args passed into the test frameworks #1917

DonJayamanne · 2018-06-09T01:30:21Z

This pull request:

Has a title summarizes what is changing
Includes a news entry file (remember to thank yourself!)
Has unit tests & code coverage is not adversely affected (within reason)
Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)

codecov · 2018-06-09T05:03:46Z

Codecov Report

Merging #1917 into master will increase coverage by 0.28%.
The diff coverage is 90.8%.

@@            Coverage Diff             @@
##           master    #1917      +/-   ##
==========================================
+ Coverage   74.74%   75.03%   +0.28%     
==========================================
  Files         302      307       +5     
  Lines       13721    14058     +337     
  Branches     2436     2493      +57     
==========================================
+ Hits        10256    10548     +292     
- Misses       3330     3373      +43     
- Partials      135      137       +2

Impacted Files	Coverage Δ
src/client/common/platform/types.ts	`100% <ø> (ø)`	⬆️
...lient/unittests/nosetest/services/parserService.ts	`86.41% <100%> (+0.16%)`	⬆️
...nt/unittests/nosetest/services/discoveryService.ts	`100% <100%> (+8.33%)`	⬆️
.../application/diagnostics/applicationDiagnostics.ts	`100% <100%> (ø)`	⬆️
src/client/unittests/serviceRegistry.ts	`97.72% <100%> (+0.66%)`	⬆️
.../client/unittests/pytest/services/parserService.ts	`75.72% <100%> (ø)`	⬆️
src/client/unittests/common/runner.ts	`91.17% <100%> (+0.85%)`	⬆️
src/client/unittests/types.ts	`100% <100%> (ø)`	⬆️
...ient/unittests/pytest/services/discoveryService.ts	`100% <100%> (+4.34%)`	⬆️
...lient/unittests/unittest/services/parserService.ts	`97.67% <100%> (-0.06%)`	⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23da67a...4d600ec. Read the comment docs.

d3r3kk

Looks pretty good, sorry it took me so long to complete my review. I have a few things that should be looked at, but for the most part it looks just fine (and I love the doi being added here!).

On the subject of dependency of injection, I see a common pattern where we simply inject the entire ServiceContainer into the constructor of our classes.

Question for you here:
Wouldn't we want to inject the exact containers we need for this class and no more?

For instance:

constructor(
  @inject(IArgumentsHelper) private argsHelper: IArgumentsHelper,
  @inject(ITestRunner) private testRunner: ITestRunner,
  ...etc...
  )

?

d3r3kk · 2018-06-13T21:22:51Z

src/client/activation/downloader.ts

@@ -58,14 +58,14 @@ export class AnalysisEngineDownloader {
    private async downloadFile(location: string, fileName: string, title: string): Promise<string> {
        const uri = `${location}/${fileName}`;
        this.output.append(`Downloading ${uri}... `);
-        const tempFile = await createTemporaryFile(downloadFileExtension);
+        const tempFile = await this.fs.createTemporaryFile(downloadFileExtension);


Love it - temp file that can be disposed to clean itself up, great.

d3r3kk · 2018-06-13T21:29:01Z

src/client/unittests/common/argumentsHelper.ts

+import { IArgumentsHelper } from '../types';
+
+@injectable()
+export class ArgumentsHelper implements IArgumentsHelper {


Is this a bespoke version of something akin to getopt? Could we just make use of a known library that would do this for us? Something that would be extensible enough to add whatever custom processing we need to apply perhaps.

Too late in the game to add new deps

I don't see the need to create nor use something generic, as this approach of parsing args is a bad approach. But this is what we have today in terms of running tests.

d3r3kk · 2018-06-18T18:48:19Z

src/client/unittests/nosetest/services/argsService.ts

+        if (Array.isArray(argumentToRemoveOrFilter)) {
+            argumentToRemoveOrFilter.forEach(item => {
+                if (OptionsWithArguments.indexOf(item) >= 0) {
+                    optionsWithoutArgsToRemove.push(item);


Did you mean to push the value into optionsWithArgsToRemove here?

(Might want to make the names less similar here to avoid this in future, nonArgOptions, argOptions, something like that).

d3r3kk · 2018-06-18T18:56:16Z

src/client/unittests/pytest/services/argsService.ts

+        if (Array.isArray(argumentToRemoveOrFilter)) {
+            argumentToRemoveOrFilter.forEach(item => {
+                if (OptionsWithArguments.indexOf(item) >= 0) {
+                    optionsWithoutArgsToRemove.push(item);


optionsWithArgsToRemove again?

DonJayamanne · 2018-06-18T22:27:22Z

@d3r3kk Please re-review.

d3r3kk

I think that covers everything, we can discuss the ServiceContainer/DependencyInjection comment I made separately as a team at the onset of an upcoming sprint (see issue #1999).

DonJayamanne changed the title ~~WIP - Update logic used to parse the args passed into the test frameworks~~ Update logic used to parse the args passed into the test frameworks Jun 11, 2018

DonJayamanne requested review from brettcannon and d3r3kk June 13, 2018 21:19

brettcannon assigned d3r3kk Jun 18, 2018

DonJayamanne force-pushed the issueWIPUnitTestArgsService branch 2 times, most recently from 5ccafa5 to 0bd432e Compare June 18, 2018 19:55

d3r3kk suggested changes Jun 18, 2018

View reviewed changes

d3r3kk approved these changes Jun 18, 2018

View reviewed changes

DonJayamanne added 15 commits June 19, 2018 08:31

Refactor unit test discovery

7d13eaf

test

50ff8a8

Unit tests

7ced68f

Update logic used to parse the args passed into the test frameworks

7a5930e

fix tests

7af4ba4

Fix tests

de073ad

Fix path to test files

f6fa080

Fix tests

4d6b928

Fix more tests

c70a33e

Fix pytest tests

4666bb2

Fix tests

92e48a0

More fixes

a8e2aee

Bug fix

df995fb

fix code review comments

e50974a

add missing awaits

b33180b

DonJayamanne force-pushed the issueWIPUnitTestArgsService branch from ac5ee79 to b33180b Compare June 19, 2018 15:33

Create tests for filtering of arguments

4d600ec

DonJayamanne closed this Jun 19, 2018

DonJayamanne reopened this Jun 19, 2018

DonJayamanne merged commit e585a15 into microsoft:master Jun 19, 2018

DonJayamanne deleted the issueWIPUnitTestArgsService branch June 20, 2018 03:13

DonJayamanne mentioned this pull request Jul 21, 2018

Running a specific nose test fails when using the '--where' setting in nosetestArgs in settings.json #1722

Closed

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

lock bot unassigned d3r3kk Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update logic used to parse the args passed into the test frameworks #1917

Update logic used to parse the args passed into the test frameworks #1917

DonJayamanne commented Jun 9, 2018

codecov bot commented Jun 9, 2018 •

edited

Loading

d3r3kk left a comment

d3r3kk Jun 13, 2018

d3r3kk Jun 13, 2018

DonJayamanne Jun 18, 2018

d3r3kk Jun 18, 2018

d3r3kk Jun 18, 2018

d3r3kk Jun 18, 2018

DonJayamanne commented Jun 18, 2018

d3r3kk left a comment

Update logic used to parse the args passed into the test frameworks #1917

Update logic used to parse the args passed into the test frameworks #1917

Conversation

DonJayamanne commented Jun 9, 2018

codecov bot commented Jun 9, 2018 • edited Loading

Codecov Report

d3r3kk left a comment

Choose a reason for hiding this comment

d3r3kk Jun 13, 2018

Choose a reason for hiding this comment

d3r3kk Jun 13, 2018

Choose a reason for hiding this comment

DonJayamanne Jun 18, 2018

Choose a reason for hiding this comment

d3r3kk Jun 18, 2018

Choose a reason for hiding this comment

d3r3kk Jun 18, 2018

Choose a reason for hiding this comment

d3r3kk Jun 18, 2018

Choose a reason for hiding this comment

DonJayamanne commented Jun 18, 2018

d3r3kk left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2018 •

edited

Loading