search

fsprojects-archive / zzarchive-VisualFSharpPowerTools

[ARCHIVED] Power commands for F# in Visual Studio
http://fsprojects.github.io/VisualFSharpPowerTools/
Apache License 2.0
310 stars 77 forks source link

Fixes to make sure that VFPT color scheme updates properly when VS theme is changed #1469

Closed surban closed 7 years ago

surban commented 7 years ago

Fixes #1359

surban commented 7 years ago

Thanks for the review. I made the changes you requested.

dungpa commented 7 years ago

The changes look good. Unfortunately, a few unit tests fail with NRE on Windows.

Here are the relevant lines:

https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/FSharp.Editing.VisualStudio.Tests/SymbolClassifierTests.fs#L19

https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/FSharp.Editing.VisualStudio.Tests/SymbolClassifierTests.fs#L19

They can be fixed by providing suitable mocks for those values. See https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/FSharp.Editing.VisualStudio.Tests/Mocks.fs#L21-L43 for an example.

Would you mind to adjust unit tests? Thank you.

surban commented 7 years ago

I tried to add a mock for ClassificationColorManager, however I still get null reference exceptions related to this type. I am not familiar with Foq. Can you help?

dungpa commented 7 years ago

Sorry, the test infra is really brittle.

You should create the mock objects in VsTestBase like others https://github.com/surban/VisualFSharpPowerTools/blob/dee4d51707e28339a29b8db07577229def44ab3a/tests/FSharp.Editing.VisualStudio.Tests/VsTestBase.fs#L38-L45 or remove this Dispose call https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/FSharp.Editing.VisualStudio.Tests/SymbolClassifierTests.fs#L45.

By the way, the similar change should be done for https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/FSharp.Editing.VisualStudio.Tests/PrintfSpecifiersUsageTaggerTests.fs#L17.

dungpa commented 7 years ago

I don't know why Foq fails to initialize ClassificationColorManager instances (perhaps it's a concrete class without virtual members). You could comment out SymbolClassifierTests for now.

surban commented 7 years ago

The problem was that UpdateColors was not virtual. The mocks are working now.

dungpa commented 7 years ago

Great work. Thanks for your persistence.

vasily-kirichenko commented 7 years ago

Published in 2.5.5.

vasily-kirichenko commented 7 years ago

@surban do you mind to fix #1479? If you don't, please make a PR which rollbacking this PR.