I like the idea of reusing CSV tests datasets.

nit: The only thing that concerns me a bit here is that we are mixing resolutions from three different places (CSV indices, AnalyzerTestUtils lookup resolution and an empty EnrichResolution), making it harder to understand what data we have when we write new tests, or how to add new datasets.

Long term, it would be good to get all of them from a single place, taking as a single source the datasets defined in CsvTestsDataLoader.

This is for a follow-up PR for sure.

test.csv file doesn't seem to exist, that's the reason for the CI failures.
I tried to add an empty file with that name and the tests seem to pass, but I don't know if that was the intention (or if you had some additional data)

Some APIs are forbidden in ES (eg. java.io.File), you'll have to replace them (Eg. with java.nio.file) to make the CI happy.

You can check API usage with

./gradlew :x-pack:plugin:esql:forbiddenApisTest

it will give you a full list of violations

nit: It would be good to have some javadocs here, explaining the difference between goldenTestToString and goldenTestNodeString

This code doesn't exist anymore but, I added some docs to the relevant format enum.

Maybe it's irrelevant, but instead of skipping, we could add a constant placeholder (eg. "$NameId$"), just to distinguish the cases where this element is present vs absent.

I've changed this to use a # prefix. This way, I'm removing them in the golden tests using regex, as suggested by @astefan, but we don't lose this information.

-Dgolden.cleanactuals should be -Dgolden.cleanactual

Nice catch, thanks!

What's with the inline comment? This code style is inconsistent with the rest of the code in ESQL (maybe ES itself, not sure).

My way of avoiding Boolean blindness :) Any way, I refactored this code, and the new version uses an enum instead.

Why are you modifying all these core classes of ESQL for something that is inherently testing related?
This is production code that should be transparent to whatever testing infrastructure needs.

I've refactored the code so the changes to production classes isn't as bad, but some classes their production code had to change since they truncated their lists. I opted to make the old nodeString method final, to make implemented who wish to override this implementation take the new format enum, as it would ensure that future classes would be aware of the requested format, and also made sure I haven't missed any cases.

As for the named identifiers, I'm removing those via regex instead of doing in production, so that code is much shorter now!

@astefan I think here it would have been safer to toString() final to ensure all implementers are updated and pass the format to recursive call correctly, but I've avoided that for now to minimize production classes changes, as there's 28 classes to update at least!

@luigidellaquila Since I changed the name to already be wrapped with `, I had to fix this.

@luigidellaquila Since I added a new column with - in it, I changed this.

It makes sense.
I think we never get quoted column names here so we should be OK for now, but maybe it makes sense to add an && name.startsWith("`") == false just to be sure.

Done.

I'm afraid the CI won't accept classes that extend RunListener directly

It's actually dumber than that: it thinks it's a test class because it has method that start with test 🤦. Apparently, making this a nested static class works though.

I would expand on this idea by encouraging to write additional unit tests to check for some specific things that cannot be verified through the plan String representation. In these optimization steps we don't always check only the string output of the resulting plan.

For example, LogicalPlanOptimizerTests:355:

assertThat(agg.aggregates().get(0).id(), equalTo(Expressions.attribute(agg.groupings().get(0)).id()));

LogicalPlanOptimizerTests:548:

        assertWarnings(
            "No limit defined, adding default limit of [1000]",
            "Line 2:28: Field 'b' shadowed by field at line 2:47",
            "Line 2:9: Field 'b' shadowed by field at line 2:47"
        );

And there are also some inline stats related tests which, due to the specific characteristics of this command, they have an additional "mocking" step to help with some more advanced checks. See LogicalPlanOptimizerTests.testDoubleInlineStatsPrunning_With_MV_Functions.

Done.

Pass max width and max props values as parameters to NodeStringFormat enum values and here and down below use that enum's value directly without this switch here and the one below.

Something like this

public enum NodeStringFormat {
        /** No list truncation, no line breaks due to string width. */
        FULL(Integer.MAX_VALUE, Integer.MAX_VALUE),
        /** List truncation and line breaks due to string width applied. */
        LIMITED(TO_STRING_MAX_PROP, TO_STRING_MAX_WIDTH);

        private int toStringMaxPropsCount;
        private int toStringMaxWidth;

        NodeStringFormat(int toStringMaxPropsCount, int toStringMaxWidth) {
            this.toStringMaxPropsCount = toStringMaxPropsCount;
            this.toStringMaxWidth = toStringMaxWidth;
        }
    }

Ah, of course! Thank you!

It's not clear to me what happens if the expected output file doesn't exist and in fact it should. I see in this piece of code that it's auto-generated if it doesn't exist.
Shouldn't there be the case that tells the author of the test to "generate the output file first or use this option to auto-generate it"? Or there shouldn't be a need for this and always auto-creating it is fine?

It's always created if it doesn't exist. From the README:

The test runner will pick up the "orphan" test and create the required outputs (see next section)
based on the test name.
You can also specify which plans you want to verify (all plans are output by default)

Since this is called a Stage, I think it would be more appropriate to have the values named as ANALYSIS, LOGICAL_OPTIMIZATION, PHYSICAL_OPTIMIZATION, LOCAL_PHYSICAL_OPTIMIZATION.

Done.

I don't have experience with this low-level way of dealing with tests, but gut-feeling wise I'd say the ES code base should have something for finding out test names, test classes names etc.

I quickly searched and I found that ESTestCase has something called getTestName(). I'd look into that to see maybe reinventing the wheel here is not needed.

Do you maybe have an alternative to "Bulldoze", something more techy/IT-specific? I'd resonate more with a word like that. But, if you feel strongly about this one, we can keep it.

This was just the term we used at my previous place :) Renamed to overwrite.

I would have used the logger everywhere if I were to write such a test class. Why the use of System.err.println?

This follows the printing semantics of ReproduceInfoPrinter, which also writes to System.err.println: https://github.com/elastic/elasticsearch/blob/main/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java#L113

If I were to search and double check the test itself and the test output file, I think it would have been easier for this folder to have the name of the test itself, meaning testRoundToTransformToQueryAndTagsWithCustomizedUpperLimit no underscores anywhere, just the same name as the test. In the IDE I would simply copy the test name and then search for the file with the same name (pasted from the test class).

Also, tbh this expected file is much much harder to grasp and reason about than the actual test and imho this is not a good example of golden testing being a better alternative. The file is huge I don't humanly see how someone would scroll the entire thing and double check it.

• Renamed the paths to use camelCase instead of underscore_case.
• I agree that some of the golden tests end up with a huge amount of filler, rendering them useless. I think it might be salvageable by allowing tests to specify how the output is being printed: for example, in this case most of the lines are just from the round_to buckets being printed. This test would still have merit even if it ignored that portion, but that's probably a task for another PR! I've remove all the tests that ended up in huge outputs.