Merge UpCodes improvements #87

marchello2000 · 2018-08-30T22:36:18Z

The following improvements have been made, we'd like to merge them back into the main repo.

Nuget package has been created
AppVeyor build has been configured
groupByModel parameter (see below) has been added
Numerous UI improvements, such as
- Better error messaging when test model isn't found
- Auto expand failing tests
- Summarize how many tests passed/failed/etc
- Show tests without a category
- Ability to clear console output
- Clear indication when model files for tests can't be found
Ability to specify a wildcard for the model filename
All console messages are marshaled back from the test and displayed by the runner
Test completion information is displayed in the console out as soon as the test itself is completed

I am happy to work with you to transfer ownership of the nuget package/appveyor build

* Tests in same fixture with same model path will run with the same model (without reopening the model) * Additional fixes: - Remember the directory for file pickers - Automatically set the working/results directory - Automatically infer the version of Revit to use from the assembly (based on the RevitAPI linked against) - Clean up a bunch of stale variables, update parts to modern C# standards - Fix by category grouping to show tests that aren't in a category

Group tests by model with the `GroupByModel` param

Nuget package definition

… is not found

Fix the test runner such that it spits out an error when a text model…

* Show how many tests pass/fail after a test run * Auto expand tree with failing tests

Various UI improvements:

…dicator

* Add `ERROR` and `WARNING` to appropriate WriteLines to clearly highlight when something goes wrong * Compact the UI a bit and give more room to console logger * Clearly state how many tests pass/fail/skip in the last run in the console log for the gui

Improvement for logging

Properly fail tests whose model isn't found

The way it's implemented is as follows: 1. During assembly load, check if a test has a wildcard for the model, e.g. ``` [TestModel(@"C:\RevitModels\test_models\*.rvt")] ``` or ``` [TestModel(@"C:\RevitModels\test_models\*.rfa")] ``` 2. Enumerate all files that match the wildcard in the specified directory 3. For each model file found, create a separate entry for the current test with this model

Add ability to specify wildcard for a model file name

Add error when no model files are found with a given wildcard

In case Revit doesn't terminate within a timeframe (45s) it is force terminated after test run is complete (only works for -continuous right now)

Add a timeout to Revit termination

* Use Nugets for Revit dependencies (e.g. RevitAPI.dll and RevitAddingUtility.dll) * Remove platform architecture warnings * Remove some unused variables and other warnings from build * Remove double definition of NUnitResultTypes * Add ability to override Revit installation enumeration during UnitTests for CI machines with Revit * Update Nuget package and add AppVeyor.yaml file

Automated build with AppVeyor * Use Nugets for Revit dependencies (e.g. RevitAPI.dll and RevitAddingUtility.dll) * Remove platform architecture warnings * Remove some unused variables and other warnings from build * Remove double definition of NUnitResultTypes * Add ability to override Revit installation enumeration during UnitTests for CI machines with Revit * Update Nuget package and add AppVeyor.yaml file

mjkkirschner · 2018-09-05T22:46:55Z

src/Applications/RTFRevit/ConsoleOutInterceptor.cs

+        void OnErrorOutLine(string text);
+    }
+
+    public class ConsoleOutInterceptor : IDisposable


can you add summary tags for this public class and public methods?

mjkkirschner · 2018-09-05T22:48:15Z

src/Applications/RTFRevit/RTFRevit.csproj

-    <Reference Include="RevitAPI">
-      <HintPath>$(REVITAPI)\RevitAPI.dll</HintPath>
+    <Reference Include="RevitAPI, Version=17.0.0.0, Culture=neutral, processorArchitecture=AMD64">
+      <HintPath>..\..\packages\Revit-2017.1.1-x64.Base.2.0.0\lib\net452\RevitAPI.dll</HintPath>


where is this nuget package from? Is this an official Autodesk distribution? Kind of odd that this is a 2019 branch of RTF but this is using a 2017 package?

I wish there was an official Autodesk channel, but I haven't seen any. Perhaps you know?
I think the whole 2017 vs 2019 branch is a bit weird as I don't see much changes Revit wise (but I also haven't deeply inspected the diff). This version of RTF works fine for 2017, 2018, and 2019.
There aren't any API's that RTF uses that are not present in 2018/2019, so I picked 2017 to cover the most versions for users. And Revit seems to happily rebind to the current version of RevitAPI.dll et.al. during its assembly resolution (which is nice)
But yes, strange. I can add a comment to the csproj
Btw, the reason I need this is to enable appeveyor CI build

Hi @marchello2000 I'm really sorry for no reply after such a long time. But I think it's better to use $(REVITAPI) path here, the package you use from nuget website may be a little risky. If you don't want to use the installed Revit, you can add a extern folder to $(REVITAPI) like
$(SolutionDir)..\lib\Revit $(RevitVersionNumber)\net47.
When CI build, you can download the RevitAPI you want to use and place them in this folder.
If you have no time for this, I would help you merge this PR and fix this problem later.

@ZiyunShang Thanks for merging, the PR. I see you already merged the PR for $(REVITAPI) change, let me comment on there as I think it's more relevant

mjkkirschner · 2018-09-05T22:49:10Z

src/Applications/RTFRevit/RevitTestClient.cs

+        }
+
+        /// <summary>
+        /// Sends a message with a line of console text that the text produces


I have read this 3 times and am still confused 😉

haha, sorry, yeah, this is not comprehensible - let me fix that!

mjkkirschner · 2018-09-05T22:50:07Z

src/Applications/RTFRevit/packages.config

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="utf-8"?>
+<packages>
+  <package id="Revit-2017.1.1-x64.Base" version="2.0.0" targetFramework="net47" />


same question as above regarding this package.

mjkkirschner · 2018-09-05T22:52:48Z

src/Applications/RevitTestFrameworkGUI/RunnerViewModel.cs

+        private bool workingDirSetByUser = false;
+        private bool resultsFileSetByUser = false;
+
+        int passedTestCount;


access modifier please.

Done, thank you!

mjkkirschner · 2018-09-05T22:53:20Z

src/Applications/RevitTestFrameworkGUI/RunnerViewModel.cs

+            }
+        }
+
+        public int PassedTestCount


please add summaries for the public properties.

mjkkirschner · 2018-09-05T22:53:31Z

src/Applications/RevitTestFrameworkGUI/RunnerViewModel.cs

+            }
+        }
+
+        public bool GroupByModel


mjkkirschner · 2018-09-05T22:56:51Z

src/Applications/RevitTestFrameworkGUI/TextBoxOutputter.cs

+
+                        tr = new TextRange(textBox.Document.ContentEnd, textBox.Document.ContentEnd);
+                        tr.Text = line;
+                        if (line.ToLower().Contains("error:"))


a small thing - would be nice if these could be referred to throughout the code using an enum or const.

good point, thanks! will add consts

mjkkirschner · 2018-09-05T23:00:50Z

src/Framework/Runner/DataTypes.cs

+        /// <summary>
+        /// Version of Revit referenced by the assembly
+        /// </summary>
+        public string ReferencedRevitVersion { get; set; }


interesting - do you have a use case for this yet?

Yeah, this is used in the GUI to automatically select the "most appropriate" Revit version installed - a small convenience

mjkkirschner · 2018-09-05T23:01:42Z

src/Framework/Runner/AssemblyLoader.cs

@@ -87,76 +97,128 @@ public static bool ReadFixture(Type fixtureType, IAssemblyData data, string work
        public static bool ReadTest(MethodInfo test, IFixtureData data, string workingDirectory)
        {
            //set the default modelPath to the empty.rfa file that will live in the build directory


is this comment still valid?

ah, gotta move it, thanks!

mjkkirschner · 2018-09-05T23:08:24Z

src/Framework/Runner/DataTypes.cs


 namespace RTF.Framework
 {
    [Serializable]
    public class AssemblyData : IAssemblyData
    {
        private bool? _shouldRun = true;
-        private ObservableCollection<ITestGroup> _sortingGroup;
+        private bool _isNodeExpanded;


style thing - I think in new .net code we prefer to not use _ notation for fields - generally we just name the field camelCase and the property PascalCase.

I agree with that, was just trying to follow "when in Rome" rule for consistency. But I am happy to fixup the whole file if you prefer. Let me know

mjkkirschner · 2018-09-05T23:09:22Z

src/Framework/Runner/DataTypes.cs

+                }
+                catch
+                {
+                    // Nothing to do, just say the model isn't there (probably a wildcard in the file name


should this be logging something?

I don't think so, this is used by the UI to highlight tests that have a missing model, so there is a feedback loop there already. the reason I had to change it was because the path now can be a wildcard which File.Exists doesn't like

mjkkirschner · 2018-09-05T23:10:11Z

src/Framework/Runner/Interfaces.cs

@@ -199,7 +200,7 @@ public interface IRunner
        /// <summary>
        /// Setup tests. Precedes a call to RunAllTests.


now that this returns a bool can you update this summary?

mjkkirschner · 2018-09-05T23:12:20Z

src/RevitTestFramework.nuspec

+        <tags>revit, autodesk, revit test framework</tags>
+    </metadata>
+    <files>
+        <file src="..\bin\AnyCPU\Release\Microsoft.Practices.Prism.dll" target="tools" />


⚠️
I do not think we should be distributing these dlls directly in this package. They have license terms that users must agree to outside of usage here - instead shouldn't this package have dependencies on those packages like nunit, prism etc...?

I agree with this 100%. The problem is that it's quite a big refactor to make RTF use all the libraries that are currently committed to the repo as nugets. I am happy to try to tackle this after this PR goes in, but I would say outside the scope of this PR.

@mjkkirschner I have addressed this by using Fody.Costura and merging the needed bits into the resultant RTF assembly. This way, these files (prism, nunit, etc) don't have to be distributed with this nuget.

mjkkirschner · 2018-09-05T23:15:56Z

@marchello2000 - Thanks for this work, lots of good improvements. I have a few comments, most are not that important but I think a few require changes before this can be merged.

Also - It appears to me there are a lot of new features here but no new tests? Not that RTF was particularly well tested itself (AFAIK) but if possible we should grow the tests to cover the new features.

marchello2000 · 2018-09-05T23:43:19Z

@mjkkirschner : thank you very much for review - I know it was a bunch of code!
I am going through comments and making changes. Will push an update in a the next day or two, time permitting.

I spend a heck of a time even understanding how to get the tests to run (since RTF doesn't use standard nunit nuget). I do have it running now. I will try to add some more tests - the problem is that the surface I changed wasn't really tested before at all, so it will probably require a few new mocks - I shall look into this.

Thanks again, updates coming soon

lazy02 · 2018-09-06T00:21:56Z

What's you send to me? 来自魅蓝 Note6

…

-------- 原始邮件 -------- 发件人：Mark Vulfson <notifications@github.com> 时间：2018年9月6日 08:16 收件人：DynamoDS/RevitTestFramework <RevitTestFramework@noreply.github.com> 抄送：Subscribed <subscribed@noreply.github.com> 主题：Re: [DynamoDS/RevitTestFramework] Merge UpCodes improvements (#87)

@marchello2000 commented on this pull request. In src/Framework/Runner/DataTypes.cs: > [XmlIgnore] public bool ModelExists { - get { return ModelPath != null && File.Exists(ModelPath); } + get + { + bool modelExists = false; + + try + { + modelExists = (ModelPath != null) && File.Exists(ModelPath); + } + catch + { + // Nothing to do, just say the model isn't there (probably a wildcard in the file name I don't think so, this is used by the UI to highlight tests that have a missing model, so there is a feedback loop there already. the reason I had to change it was because the path now can be a wildcard which File.Exists doesn't like — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread. {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/DynamoDS/RevitTestFramework","title":"DynamoDS/RevitTestFramework","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in ***@***.*** commented on #87"}],"action":{"name":"View Pull Request","url":"#87 (comment)"}}} [ { ***@***.***": "http://schema.org", ***@***.***": "EmailMessage", "potentialAction": { ***@***.***": "ViewAction", "target": "#87 (comment)", "url": "#87 (comment)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { ***@***.***": "Organization", "name": "GitHub", "url": "https://github.com" } }, { ***@***.***": "MessageCard", ***@***.***": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": ***@***.*** commented on 87", "sections": [ { "text": "", "activityTitle": "**Mark Vulfson**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": ***@***.***", "facts": [ ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "#87 (comment)" } ], ***@***.***": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", ***@***.***": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 374427143\n}" } ], "themeColor": "26292E" } ]

* Ensure all new methods/classes have XML docs * Ensure all memebers have access modifiers

Using Costura to merge the needed binaries into the RTF assemblies such that they don't need to be redistributed without correct nuget usage

PR Feedback

marchello2000 · 2018-09-20T19:23:03Z

@mjkkirschner : sorry for the delay. I have pushed the feedback changes to this PR. If you want to view just those changes: 07137c0

Aside from unit testing the changes, I believe I addressed all comments. Unit testing is tricky because the runner aspect of the codebase isn't really unittested. I will look into adding a bunch more mocks to add the tests. But that should be a separate PR as a) I am not sure when I'll get to it and b) it's already getting quite huge.

Note that I have removed the .yml file for the appveyor build. But I can work with your and your team to add it such that it's being built from your appveyor account.

Let me know how you want to proceed

mjkkirschner · 2018-10-03T02:05:14Z

@QilongTang @smangarole @ZiyunShang is there any problem merging this in now or should we wait until after any cross team handoffs are made - I don't want to distrub the builds if they consume master RTF or something like that?

mjkkirschner · 2018-10-03T02:12:08Z

Also note that if this is merged we must take on the work of fixing up the nugets - I don't think it is appropriate to merge these dlls - like prism for too long without at-least including their licenses in this repo.

mjkkirschner · 2018-10-04T17:19:44Z

@marchello2000 thanks for the hardwork, I'm planning to get this in soon or some version of it. I'm still a bit confused about the use of frody.costura- can you tell me what assemblies are being merged to the revitTestFramework assemblies and for what purpose?

marchello2000 · 2018-10-08T02:49:48Z

@mjkkirschner : apologies for the delay.
The Costura plugin for Fody basically performs ILMerge on the final assembly. This way, for example, Microsft.Practices.Prism.dll doesn't need to go into the Nuget because it's "linked"/"merged" directly into the RevitTestFrameworkGUI.exe. (https://github.com/DynamoDS/RevitTestFramework/pull/87/files#diff-500dbf6051225bb795faa5da2c76346fR19)

I also added a licenses.txt file into the repo & nuget to address licensing concerns. (https://github.com/DynamoDS/RevitTestFramework/pull/87/files#diff-0714f5678ce40fb39fb7f5631869fc7fR1)

Garrett-R · 2018-11-28T20:44:10Z

Hey, just curious, think you folks will have any time to continue with this code review?

Mark Vulfson and others added 30 commits May 16, 2018 11:01

Nuget package definition

e432ff5

Merge pull request #1 from marchello2000/mark/groupbymodel

4010ab8

Group tests by model with the `GroupByModel` param

Merge pull request #2 from marchello2000/mark/nuget

a536c9f

Nuget package definition

Fix the test runner such that it spits out an error when a text model…

67a247b

… is not found

Merge pull request #3 from marchello2000/mark/better_error_messages

f35ce3d

Fix the test runner such that it spits out an error when a text model…

Various UI improvements:

58261f9

* Show how many tests pass/fail after a test run * Auto expand tree with failing tests

Merge pull request #4 from marchello2000/mark/failing_tests_indicator

fafa84f

Various UI improvements:

fixup! Merge pull request #4 from marchello2000/mark/failing_tests_in…

42612ba

…dicator

Clarity improvements:

e22d019

* Add `ERROR` and `WARNING` to appropriate WriteLines to clearly highlight when something goes wrong * Compact the UI a bit and give more room to console logger * Clearly state how many tests pass/fail/skip in the last run in the console log for the gui

Nuget version update

31ebdc6

Add ability to marshal console logs from unit test

931829d

Spit out information to console as the test complete

f8704e4

Add ability to clear in the console box

8e5a4f0

Update nuspec version and readme

82cc0e2

Merge pull request #5 from upcodes/mark/marshal_console_out

0882fb6

Improvement for logging

Add missing empty.rfa file to the nuget package

d9d5461

Porting fix for DynamoDS#71

93d445d

Properly fail tests whose model isn't found

47151cc

Merge pull request #6 from upcodes/mark/proper_fail_no_model

30ea382

Properly fail tests whose model isn't found

Fix null deref in console mode

9668e0f

Merge pull request #7 from upcodes/mark/regex_model_path

ca86a3a

Add ability to specify wildcard for a model file name

Add proper error handling for wildcard models

92409d3

Add error when no model files are found with a given wildcard

Initialize output redirector earlier to catch command line arguments

86331c9

Add a timeout to Revit termination

9f6ea40

In case Revit doesn't terminate within a timeframe (45s) it is force terminated after test run is complete (only works for -continuous right now)

Update README.md

1d8d105

Merge pull request #8 from upcodes/mark/timeout_on_exit

8add7ec

Add a timeout to Revit termination

mjkkirschner reviewed Sep 5, 2018

View reviewed changes

Mark Vulfson and others added 4 commits September 20, 2018 10:21

PR Feedback:

26214e0

* Ensure all new methods/classes have XML docs * Ensure all memebers have access modifiers

Update readme

255e459

PR Feedback: do not distribute nunit/prism/etc libraries

a83f7d1

Using Costura to merge the needed binaries into the RTF assemblies such that they don't need to be redistributed without correct nuget usage

Merge pull request #11 from upcodes/mark/prfixups2

07137c0

PR Feedback

Adding license info for used dlls

7b1b395

ZiyunShang merged commit 53a4d9e into DynamoDS:Revit2019 Mar 22, 2019

marchello2000 mentioned this pull request Mar 25, 2019

Update RevitAPI reference Path #88

Merged

		@@ -199,7 +200,7 @@ public interface IRunner
		/// <summary>
		/// Setup tests. Precedes a call to RunAllTests.

Merge UpCodes improvements #87

Merge UpCodes improvements #87

Conversation

marchello2000 commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Sep 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Sep 5, 2018

marchello2000 commented Sep 5, 2018

lazy02 commented Sep 6, 2018 via email

marchello2000 commented Sep 20, 2018

mjkkirschner commented Oct 3, 2018

mjkkirschner commented Oct 3, 2018

mjkkirschner commented Oct 4, 2018

marchello2000 commented Oct 8, 2018

Garrett-R commented Nov 28, 2018 • edited

mjkkirschner Sep 5, 2018 •

edited

Garrett-R commented Nov 28, 2018 •

edited