Avoid duplication in package assembly list

[mmisol]

Purpose

Fixes the issue described here https://forum.dynamobim.com/t/package-contents-listed-twice-in-the-package-manager/48994. Tracked as DYN-2602.

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

@aparajit-pratap @QilongTang

FYIs

@mjkkirschner

[mmisol]

There are basically two things that were fixed:

1. The call to EnumerateAssembliesInBinDirectory was replaced by using LoadedAssemblies. This avoids re-adding assemblies to LoadedAssemblies.
2. The assemblies list is materialized. Before these changes, the call to EnumerateAssembliesInBinDirectory was made each time the enumerable was enumerated, which happened once per event handler for PackageLoaded. That caused triplication of the libraries for me on latest master.

[aparajit-pratap]

@mmisol you mentioned that EnumerateAssembliesInBinDirectory was being called in a couple of other places. Do those need to be replaced with LoadedAssmeblies as well?

[mmisol]

Yes, you are right. What I was seeing in the stack trace was actually this line of code being executed when the enumeration was materialized. This happened in two event handlers of OnPackagesLoaded, one in DynamoModel.cs(978) and another in EngineController.cs(521).

[QilongTang]

@mmisol So I assume whichever event handler called second time, with this fix, we will no longer re-adding assemblies to LoadedAssemblies. Do we want to create a test by mocking up the situation and check the end LoadedAssemblies?

[mmisol]

Yes, that was part of the issue. The call to TryLoadPackageIntoLibrary is the first to call EnumerateAssembliesInBinDirectory and after this LoadedAssemblies should be used to read them.

I'll see to add a test. I have to think about what needs to be mocked, hopefully it's not too difficult.

[aparajit-pratap]

@mmisol could you create a JIRA issue for this for tracking purposes.

[aparajit-pratap]

@mmisol thanks for fixing this issue so quickly! Are there any tests you can add for this?

[mjkkirschner]

should the use of this enumerateAssemblies method also be changed here:

Dynamo/src/DynamoPackages/PackageLoader.cs

Line 261 in 7cbe479

LocalPackages.SelectMany(x => x.EnumerateAssembliesInBinDirectory().Where(y => y.IsNodeLibrary));

(what Aparajit asked above)

[mmisol]

@mjkkirschner That code seems to have the same problem. I'll check if it's being used somewhere.

[mmisol]

Added a test. This is ready for review @aparajit-pratap @QilongTang

@mjkkirschner About the method you mentioned, it's obsoleted and there are no usages, so I left it alone. Let me know if you would like to change ti anyway, but testing it would be difficult.

[QilongTang]

@mmisol Thank you for adding the test. LGTM. Not sure if we need to include the other spot also part of this PR, I think it is OK to not include. Oops.. Wrong button

[mmisol]

Re-thinking about what I wrote yesterday, unit testing the obsolete method should be easy. If you guys think it's safer to change it and unit test it rather than leaving as-is let me know and I can change it.

[aparajit-pratap]

Re-thinking about what I wrote yesterday, unit testing the obsolete method should be easy. If you guys think it's safer to change it and unit test it rather than leaving as-is let me know and I can change it.

I think this is good. LGTM.

[mmisol]

Is this PR good to be merged @QilongTang @aparajit-pratap @mjkkirschner ? If so, can I get an approval? Thank you

[QilongTang]

@mmisol Ship it!