Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complete InspectionTestsBase incorporation #5226

Merged
merged 103 commits into from Oct 30, 2019

Conversation

@BZngr
Copy link
Contributor

BZngr commented Oct 15, 2019

Closes #5191. This PR incorporates InspectionTestsBase into the remaining inspection tests (except one - see below). The good news is that there is net reduction of nearly 4000 lines of inspection test code thanks to the InspectionTestsBase class.

Other notes of interest:
UnreachableCaseInspectionTests:

  1. Is the only test class remaining that does not derive from **InspectionTestsBase**:
  2. The UnreachableCaseInspectionTests as-written make use of the InspectionResults.resx to identify which result(s) occur for the various test scenarios. The resource string comparisons/manipulations were used by the tests because this inspection flags more than one type of issue. So, there are a number of places in the test code that look like InspectionResults.UnreachableCaseInspection_Unreachable. These references to the resource file become name conflicts with the InspectionTestsBase.InspectionResults method. So, this particular test class could not inherit from the base class. I can look at reworking the test class, but I'm thinking this would be outside the scope of this PR.

InspectionTestsBase:
Added some overloads within this class, particularly regarding the InspectionResultsForModules method. The overloads all have to do with making it easier to introduce reference libraries when building up theVBE. The overloaded methods take either a single library identifier string or an enumerable list in addition to the module-defining tuple(s). The MockVBEBuilder uses raw strings to organize and manage the libraries - so the added overloads are consistent with that approach.

MockVBEBuilder:

  1. Added some overloads to the BuildFromModules method to support the overloads added to InspectionTestsBase.
  2. Modified public const string TestProjectName and TestModuleName to be public static string properties. This allows tests an option to modify the default names without breaking pre-existing references to the constants. This was needed/used for the UseMeaningfulNameInspectionTests (issue described below).

UseMeaningfulNamesTests:
The UseMeaningfulName inspection requires different identifiers than the MockVBEBuilder's default Project and Module names. "TestProject1" and "TestModule1" both end with a number - and therefore are flagged by the inspection. There seemed to be a couple options:
a) Simply change the default values to ones that are not flagged. or,
b) Provide some mechanism for the test subclasses to set the default name.

I chose the latter approach. This was partly motivated by the fact that at several tests use the default Project and Module names as part of there expected value for comparison. The other reason is that there may be other test classes (now or in the future) that wish to use identifiers different than the defaults if for no other reason than to remove the possibility that changes to the default name would break its tests.

So, the following changes were made:
a. Modified MockVBEBuilder Project and Module default name constants (described above).
b. Added OneTimeSetup and OneTimeTeardown attributed methods to UseMeaningfulNameInspectionTests. Since the MockVBEBuilder properties are static, the setup/teardown methods are needed to ensure that tests running subsequent to the UseMeaningfulNameInspectionTests would receive the expected default values.

@retailcoder

This comment has been minimized.

Copy link
Member

retailcoder commented Oct 18, 2019

Couldn't using resx = Rubberduck.Resources.Inspections.InspectionResults; fix the naming clash with UnreachableCaseInspectionTests?

Copy link
Contributor

MDoerner left a comment

Tests have to be isolated from each other. So, they should never depend on mutable global state like a static writable property. This could change results based on the order they are executed and would make running them in parallel impossible.

Accordingly, I would like you revert the introduction of the two new static properties on MockVbeBuilder.

public const string TestProjectName = "TestProject1";
public const string TestModuleName = "TestModule1";
public static string TestProjectName { set; get; } = "TestProject1";
public static string TestModuleName { set; get; } = "TestModule1";

This comment has been minimized.

Copy link
@MDoerner

MDoerner Oct 18, 2019

Contributor

I think this is not a good idea because it potentially causes coupling between different tests. (The statics are shared between all tests.)

I think to achieve the goal of allowing to change the project name for VBEs constructed via static helper methods on this class it would be better to simply provide new overloads allowing to pass in this data.

@@ -53,12 +54,21 @@ public class MockVbeBuilder
["ADOR"] = LibraryPathAdoRecordset
};

private static readonly Dictionary<string, Action<MockProjectBuilder>> addReferenceActions = new Dictionary<string, Action<MockProjectBuilder>>
{

This comment has been minimized.

Copy link
@MDoerner

MDoerner Oct 18, 2019

Contributor

Can weake this a dictionary of Func<MockProjectBuilder, MockProjectBuilder> please to keep the fluid nature of the builder methods?

This comment has been minimized.

Copy link
@bclothier

bclothier Oct 18, 2019

Contributor

On a different subject, I'm wondering if since we are cleaning up how references are handled why not use a enum or a static class with static members instead of string to key the references? Using a static class would make it easy for us to write the necessary data for any new reference we wish to add and have a compiler-validated type on all references?

This comment has been minimized.

Copy link
@MDoerner

MDoerner Oct 19, 2019

Contributor

I agree, what we really want here is an enum of libraries together with an overload of AddReference taking a params array of enum elements.

This comment has been minimized.

Copy link
@BZngr

BZngr Oct 19, 2019

Author Contributor

I like the idea of moving away from string keys but I think doing so should be the focus of a different PR. That type of modification will introduce changes to many test files other than inspection tests.

/// <summary>
/// Builds a mock VBE containing one project with one module and multiple libraries.
/// </summary>
public static Mock<IVBE> BuildFromModules((string name, string content, ComponentType componentType) module, IEnumerable<string> libraries)

This comment has been minimized.

Copy link
@MDoerner

MDoerner Oct 18, 2019

Contributor

How about using a params array?

public IEnumerable<IInspectionResult> InspectionResultsForModules((string name, string content, ComponentType componentType) module, string library)
=> InspectionResultsForModules(new (string, string, ComponentType)[] { module }, new string[] { library });

public IEnumerable<IInspectionResult> InspectionResultsForModules(IEnumerable<(string name, string content, ComponentType componentType)> modules, string library)

This comment has been minimized.

Copy link
@MDoerner

MDoerner Oct 18, 2019

Contributor

How about using a params array?

@BZngr

This comment has been minimized.

Copy link
Contributor Author

BZngr commented Oct 19, 2019

All comments and requested changes are addressed:

  1. UnreachableCaseInspectionTests has been updated to use base class.
  2. Reverted MockVBEBuilder static property change.
  3. Implemented other MockVBEBuilder and InspectionTestsBase overload improvement comments.
  4. MockVBEBuilder ReferenceLibrary Dictionary values are now Func<MockVBEBuilder ,MockVBEBuilder >.
@retailcoder retailcoder merged commit f747533 into rubberduck-vba:next Oct 30, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.