Dyn 3026 code coverage analysis folder #10989

RobertGlobant20 · 2020-08-11T19:02:19Z

Purpose

Increase the code coverage adding test cases for the Libraries/Analysis folder.
I created several unit tests methods using nUnit for the next classes:

Label
SurfaceData
AnalysisExtensions
Utils

Also I removed two methods from the SurfaceData.cs file since both have zero references and both are private.
The job DynamoSelfServe ran successfully in master-5.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

@QilongTang

FYIs

@alfredo-pozo

I created several test methods for increasing the code coverage in the next classes (Analysis project). - Label - SurfaceData - AnalysisExtensions - Utils Also I removed two methods from the SurfaceData.cs file since both have cero references and both are private.

Fix for make the Analysis assembly visible to the AnalysisTests project.

RobertGlobant20 · 2020-08-11T19:05:38Z

This is a screenshot about the code coverage for the Analysis project using DotCover command line:

mjkkirschner · 2020-08-11T20:05:21Z

@RobertGlobant20 did you ever get these methods to actually draw labels into the background preview?

RobertGlobant20 · 2020-08-11T20:51:20Z

@RobertGlobant20 did you ever get these methods to actually draw labels into the background preview?

@mjkkirschner No, I tried to find references to the Label class (or classes that inherit of Label class) and is only used in the Label.cs and the test class that i created, then I think there is no way to used in other places, probably the labels in the background preview is using a different class.

aparajit-pratap · 2020-08-11T22:01:54Z

test/Libraries/AnalysisTests/LabelTests.cs

+            var package = factory.CreateRenderPackage();
+
+            //Act
+            labelTest.Tessellate(package, new TessellationParameters());


What are you testing from the Tessellate call? I think the assertions below will pass even if you don't call Tessellate, wouldn't they?

@aparajit-pratap
The Tessellate method is a [IsVisibleInDynamoLibrary(false)], has no code coverage and is not used (inside the Label class o in other classes, event the whole Label class is not used), I was proposing to delete the whole Label.cs file but Michael said the next "if it's public, we cannot get rid of it as it will break semantic versioning."
You are correct, the assertions are checking the results of Label.ByPointAndString method then they don't depend of calling the Tessellate method, then is it ok if I just delete the call to the Tessellate method (of course if won't have coverage)?

It looks like the package properties are modified in Tessellate. You can check for those properties after the call to Tessellate.

@aparajit-pratap this test method was re-written due to some comments that Michael did, please review again.

aparajit-pratap · 2020-08-11T22:02:51Z

test/Libraries/AnalysisTests/LabelTests.cs

+            labelTest.Tessellate(package, new TessellationParameters());
+
+            //Assert
+            Assert.IsNotNull(labelTest);


I think this assertion should be before the call to Tessellate. Also, can you check if certain properties of Label are set in addition to checking for it to be non-null?

@aparajit-pratap
This depends of the previous comment, if I delete the call to Tessellate method there is no need to do it, otherwise I will move it before the call (see commit: 0fbc203).
Also is important to mention that Label class only have private members (no public properties).

test/Libraries/AnalysisTests/SurfaceDataTests.cs

Changes asked by Aparajit.

mjkkirschner · 2020-08-12T18:58:44Z

Hi @RobertGlobant20 - whats odd is that https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/Analysis/Label.cs#L42
should create a point in the background at least.

I re-wrote the test for the Label class since Michael asked me to use a Label inside a Code Block node, then I created 3 dyn files with one CodeBlock creating a Label, one compiling and other two with errors (on purpose) when passing parameters to the Label.ByPointAndString method. Also by Michael response I undid the changes done by Aaron in the PR #8189. Finally I applied some changes that the developer Ian Keough did like 5 years ago (HelixRenderPackage.cs -> CleanTag) but were discarded in the PR below, with the code as it is now in the Dynamo repo when creating a Label inside a Code Block it shows a strange symbol in the Background Preview. The only way to show the label text correctly was using the syntax "label:text", then Ian's discarded change was re-applied by me and now we don't need to use the weird Label syntax. #4637

I applied some changes that the developer Ian Keough did like 5 years ago (HelixRenderPackage.cs -> CleanTag) but were discarded in the PR below, with the code as it is now in the Dynamo repo when creating a Label inside a Code Block it shows a strange symbol in the Background Preview. The only way to show the label text correctly was using the syntax "label:text", then Ian's discarded change was re-applied by me and now we don't need to use the weird Label syntax. #4637

RobertGlobant20 · 2020-08-18T15:53:03Z

Hi @RobertGlobant20 - whats odd is that https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/Analysis/Label.cs#L42
should create a point in the background at least.

@mjkkirschner The tests for the Label class were rewritten and also I applied changes in the Label.cs and HelixRenderPackage.cs files, please review again.

mjkkirschner · 2020-08-18T16:12:08Z

hmm, @RobertGlobant20 it looks like this change causes the mono.sln build to fail - thats because now the analysis tests require sharpDX and WPF references.

I think we have two options.

remove the analysis tests from the mono.sln
Add those tests that require those references to the wpfvisualizationTests project instead of to the analysisTests project, so the analysis test project does not need these windows only references.

what do you think?

mjkkirschner · 2020-08-18T17:11:59Z

test/Libraries/AnalysisTests/AnalysisTests.csproj

    </ProjectReference>
+    <ProjectReference Include="..\..\..\src\Libraries\CoreNodeModels\CoreNodeModels.csproj">
+      <Project>{D8262D40-4880-41E4-91E4-AF8F480C8637}</Project>
+      <Name>CoreNodeModels</Name>


@RobertGlobant20 - this change is causing atleast some of the 600 failures you are getting.
Change this to <Private>False<Private> or in visual studio CopyLocal False, same thing.

This binary is being loaded twice, once from the /nodes folder, and now again from the /bin folder (in your PR). It should only be present in the /nodes folder.

To be safe, I would make all of these dependencies copy Local false, but thats probably not the problem.

@mjkkirschner I removed the CoreNodeModels reference from the AnalysisTests project, then there shouldn't be any problem now.
Thanks

I moved the LabelTests to the DynamoCoreWpfTests project inside the Libraries folder. I removed several dll in the AnalysisTests project that are producing erros in the mono.sln.

RobertGlobant20 · 2020-08-18T17:33:14Z

hmm, @RobertGlobant20 it looks like this change causes the mono.sln build to fail - thats because now the analysis tests require sharpDX and WPF references.

I think we have two options.

remove the analysis tests from the mono.sln

Add those tests that require those references to the wpfvisualizationTests project instead of to the analysisTests project, so the analysis test project does not need these windows only references.

what do you think?

@mjkkirschner I took the option 1, I moved the LabelTests.cs to the DynamoCoreWpfTests project inside a Libraries folder, then I removed the sharpDX and Wpf references from the AnalysisTests project.

mjkkirschner

let's wait for the tests, but otherwise looks good.

RobertGlobant20 · 2020-08-18T18:21:19Z

let's wait for the tests, but otherwise looks good.
@mjkkirschner The Tests were successful executed.
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/master/

RobertGlobant20 added 2 commits August 11, 2020 12:23

DYN-3026-CodeCoverage-AnalysisFolder

2a6ace8

Fix for make the Analysis assembly visible to the AnalysisTests project.

QilongTang approved these changes Aug 11, 2020

View reviewed changes

aparajit-pratap reviewed Aug 11, 2020

View reviewed changes

DYN-3026-CodeCoverage-AnalysisFolder-CodeReview

0fbc203

Changes asked by Aparajit.

RobertGlobant20 added 2 commits August 18, 2020 10:38

mjkkirschner reviewed Aug 18, 2020

View reviewed changes

DYN-3026-CodeCoverage-AnalysisFolder-CodeReview2

6dbdce2

I moved the LabelTests to the DynamoCoreWpfTests project inside the Libraries folder. I removed several dll in the AnalysisTests project that are producing erros in the mono.sln.

mjkkirschner approved these changes Aug 18, 2020

View reviewed changes

mjkkirschner merged commit 8ee9b0e into DynamoDS:master Aug 18, 2020

saintentropy mentioned this pull request Apr 26, 2021

DYN-2538 Generate single RenderPackage per Node #11395

Merged

12 tasks

Dyn 3026 code coverage analysis folder #10989

Dyn 3026 code coverage analysis folder #10989

Uh oh!

Conversation

RobertGlobant20 commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

FYIs

Uh oh!

RobertGlobant20 commented Aug 11, 2020

Uh oh!

mjkkirschner commented Aug 11, 2020

Uh oh!

RobertGlobant20 commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aparajit-pratap Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

RobertGlobant20 Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

RobertGlobant20 Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

RobertGlobant20 Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mjkkirschner commented Aug 12, 2020

Uh oh!

RobertGlobant20 commented Aug 18, 2020

Uh oh!

mjkkirschner commented Aug 18, 2020

Uh oh!

mjkkirschner Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

RobertGlobant20 Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

RobertGlobant20 commented Aug 18, 2020

Uh oh!

mjkkirschner left a comment

Choose a reason for hiding this comment

Uh oh!

RobertGlobant20 commented Aug 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RobertGlobant20 commented Aug 11, 2020 •

edited

Loading

RobertGlobant20 commented Aug 11, 2020 •

edited

Loading

RobertGlobant20 Aug 12, 2020 •

edited

Loading