search

DynamoDS / RevitTestFramework

Unit Testing on Revit
119 stars 81 forks source link

Merge UpCodes improvements #87

Closed marchello2000 closed 5 years ago

marchello2000 commented 5 years ago

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

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

marchello2000 commented 5 years ago

FYI @ZiyunShang @AndyDu1985 @philxia1 @QilongTang @jeremytammik These are the changes I published about here

Would love to merge these back into the main repo. Let me know if there is anything you need from me. Also, we are happy to work with you all to transfer ownership of the nuget package as well as the AppVeyor build

ZiyunShang commented 5 years ago

rtfcapture1 There seems to be something wrong with the path in the red area. I think this path should be consistent with Working Directory, right?

marchello2000 commented 5 years ago

@ZiyunShang : I will look into the model directory issue - it does make sense to be relative to the working directory.

marchello2000 commented 5 years ago

@ZiyunShang I do not understand what you mean about the paths.

The working directory is correctly taken into account. Here is the code: https://github.com/upcodes/RevitTestFramework/blob/mark/Revit2019/src/Framework/Runner/AssemblyLoader.cs#L133

Your tests in the image are using empty.rfa which will always come from the same location as RTF assembly (see https://github.com/upcodes/RevitTestFramework/blob/mark/Revit2019/src/Framework/Runner/AssemblyLoader.cs#L163) - this code didn't change.

Am I missing something?

ZiyunShang commented 5 years ago

Hi @marchello2000 . It looks like this path problem has existed before, and it's not your fault. @QilongTang @mjkkirschner I have reviewed this pr , these changes seems great. But I don't have the access to this repo, could you take a look at this?

marchello2000 commented 5 years ago

Thank you, @ZiyunShang, for reviewing!

mjkkirschner commented 5 years ago

Hi @ZiyunShang your team should now have write access to this repo :).

mjkkirschner commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 GitHub","url":"https://github.com/DynamoDS/RevitTestFramework"}},"updates":{"snippets":[{"icon":"PERSON","message":"@marchello2000 commented on #87"}],"action":{"name":"View Pull Request","url":"https://github.com/DynamoDS/RevitTestFramework/pull/87#discussion_r215463028"}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/DynamoDS/RevitTestFramework/pull/87#discussion_r215463028", "url": "https://github.com/DynamoDS/RevitTestFramework/pull/87#discussion_r215463028", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "@marchello2000 commented on 87", "sections": [ { "text": "", "activityTitle": "Mark Vulfson", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@marchello2000", "facts": [ ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "https://github.com/DynamoDS/RevitTestFramework/pull/87#discussion_r215463028" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 374427143\n}" } ], "themeColor": "26292E" } ]

marchello2000 commented 5 years ago

@mjkkirschner : sorry for the delay. I have pushed the feedback changes to this PR. If you want to view just those changes: https://github.com/DynamoDS/RevitTestFramework/pull/87/commits/07137c0393059e21e40c6cadb2476f84ca117a7a

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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