test: ignore_test suggestion

[dmyger]

No description provided.

[elhimov]

I see following meaningful changes:

1. Map is used instead of list (slice) to hold test cases.
   I see no problem with that and can apply this change in the original PR

2. TestData struct was dropped.
   I doubt that it makes code clearer. TestData was used as a kind of abstract test case, i.e. it was not used as-is in tests, but was used to generate "concrete" test case suitable for concrete function (either createIgnorePattern or ignoreFilter) in a similar ways. This PR uses "concrete" sets for createIgnorePattern and generate another "concrete" sets for ignoreFilter from it and this way seems less intuitive for me.

3. Named functions (over variable) are used instead of lambda functions.
   Frankly speaking, I don't see a big difference with the original approach. Previously any of lambda-function was the core code of corresponding test and it was quite clear what it did from the test name itself. So I see no changes in readability in the example below:

func Test_createIgnorePattern_negate(t *testing.T) {
	testCases := mapSlice(testCases_ignorePatternBasic,
		func(tc testCase_ignorePattern) testCase_ignorePattern {
			return testCase_ignorePattern{
				name:               tc.name,
				pattern:            "!" + tc.pattern,
				expectedMatches:    tc.expectedMatches,
				expectedMismatches: tc.expectedMismatches,
				expectedDirOnly:    tc.expectedDirOnly,
				expectedIsNegate:   true,
			}
		},
	)
	runTestSet_ignorePattern(t, testCases)
}

vs

func Test_createIgnorePattern_negate(t *testing.T) {
	negate := func(tc testCase_ignorePattern) testCase_ignorePattern {
			return testCase_ignorePattern{
				name:               tc.name,
				pattern:            "!" + tc.pattern,
				expectedMatches:    tc.expectedMatches,
				expectedMismatches: tc.expectedMismatches,
				expectedDirOnly:    tc.expectedDirOnly,
				expectedIsNegate:   true,
			}
		}
	testCases := mapSlice(testCases_ignorePatternBasic, negate)
	runTestSet_ignorePattern(t, testCases)
}

[elhimov]

The whole point of the above set (original name is ignoreTestData_names) is that none of them contains path separator in pattern (but contains various wildcards). This set (original name is ignoreTestData_paths) do contain path separator in pattern.
I can agree that original naming is not the best one, but the suggested one doesn't reflect the nature of the set at all.

[dmyger]

1. Map is used instead of list (slice) to hold test cases.

There is a suggestion to always use map in table driven tests: it makes the case name stand out from the structure, which makes it visually easier to understand what the case is about. And most importantly, there is a guarantee to avoid duplication of names, which helps in debugging.

2. TestData struct was dropped

The ignoreTestData structure was not deleted, it was just merged with testCase_ignorePattern to remove unnecessary data copies that do no benefit and only make the code bigger.

3. Named functions (over variable) are used instead of lambda functions.

Both of the given examples are overengineered and unreadable.

func Test_createIgnorePattern_negate(t *testing.T) {
	invertPattern := func(td *ignorePatternCase) {
		td.pattern = "!" + td.pattern
		td.isNegate = true
	}

	tc := preparePatterns(ignoreBaseTestCasesPattern, invertPattern)
	checkIgnorePattern(t, tc)
}

My suggestion is to remove explicit data copying altogether - it makes the code dirty, hiding the useful idea of 1-2 lines behind a forest of empty actions.
In the proposed version, there is a function with a speaking name that performs a simple transformation and, it is easy to understand.

Syntactic sugar in the form of mapSlice is an unnecessary construct at all, which by its presence complicates readings.

ignoreTestData_paths vs ignoreTestCaseWildcard

The name doesn't matter at all.
The main idea is that it should reflect why these cases are not joined into one cases list. This is because the first part is used for Filters and the second part is not. A name like ignoreTestDataPart2 might even be more appropriate here, up to you.

[elhimov]

There is a suggestion to always use map in table driven tests: it makes the case name stand out from the structure, which makes it visually easier to understand what the case is about. And most importantly, there is a guarantee to avoid duplication of names, which helps in debugging.

No problem. As I mentioned earlier I have no objection and have applied this change.

The ignoreTestData structure was not deleted, it was just merged with testCase_ignorePattern to remove unnecessary data copies that do no benefit and only make the code bigger.

The problem is that ignorePatternCase (test case for createIgnorePattern) contains fields that meaningless for ignoreFilter and in my opinion the confusion effect from this fact overcomes the simplification. That's why an abstract test item container was introduced which is intended to contain the minimum but yet meaningful data that is (along with transformation function) "expanded" to the desired test case for the certain function.

Both of the given examples are overengineered and unreadable.

func Test_createIgnorePattern_negate(t *testing.T) {
	invertPattern := func(td *ignorePatternCase) {
		td.pattern = "!" + td.pattern
		td.isNegate = true
	}

	tc := preparePatterns(ignoreBaseTestCasesPattern, invertPattern)
	checkIgnorePattern(t, tc)
}

My suggestion is to remove explicit data copying altogether - it makes the code dirty, hiding the useful idea of 1-2 lines behind a forest of empty actions. In the proposed version, there is a function with a speaking name that performs a simple transformation and, it is easy to understand.

I doubt that you mean that, but it seems this way we change the original test set at each transformation which requires us to keep in mind what is in origin at any time, keep test orders or something similar. In my opinion this approach complicates the readability and here "the less code" doesn't mean "the better".

I've considered all your comments and have done my best to make code friendlier to maintainer )). Please, take a look (#1096).