[Unittest] Expected error for statement error is no longer optional
#9554
Conversation
…not an empty line or a '----', should probably be fixed)
…or messages are not related
…al_error_checking
…ert *that* they error, but can't find an exact reoccurring pattern (for example when a loop is used)
…al_error_checking
… the corresponding 'duckdb_extension_load' calls
…al_error_checking
Exit code 2 |
There was a problem hiding this comment.
Have looked at the following directories. Can look at the rest another time.
.github
test/common
test/extenstion
test/fuzzer/*
test/helpers/*
test/include/*
test/issues/fuzz/*
test/issues/general/*
I stopped at tests/issues/monetdb and will finish reviewing the rest at another time
|
Thanks for the PR! I'm guessing these error messages are auto-generated right now based on the output of the queries? While I think making the error message mandatory is a good idea auto-generating all of these error messages into the tests seems problematic to me. Many of these error messages might change, which is why we in general only place part of an error message in the expected error. An example of this is e.g.: statement error
select mode()
----
Binder Error: No function matches the given name and argument types 'mode()'. You might need to add explicit type casts.
Candidate functions:
mode(DECIMAL) -> DECIMAL
mode(TINYINT) -> TINYINT
mode(SMALLINT) -> SMALLINT
mode(INTEGER) -> INTEGER
mode(BIGINT) -> BIGINT
mode(HUGEINT) -> HUGEINT
mode(FLOAT) -> FLOAT
mode(DOUBLE) -> DOUBLE
mode(UTINYINT) -> UTINYINT
mode(USMALLINT) -> USMALLINT
mode(UINTEGER) -> UINTEGER
mode(UBIGINT) -> UBIGINT
mode(DATE) -> DATE
mode(TIMESTAMP) -> TIMESTAMP
mode(TIME) -> TIME
mode(TIMESTAMP WITH TIME ZONE) -> TIMESTAMP WITH TIME ZONE
mode(TIME WITH TIME ZONE) -> TIME WITH TIME ZONE
mode(INTERVAL) -> INTERVAL
mode(VARCHAR) -> VARCHAR
This error message will change if at any point a new overload is added - which will then cause the test to fail. I would propose we instead leave all error messages that are not currently defined as empty (i.e. add the dashes below the statement error, |
This PR is quite massive.
The expected errors are generated from the actual results, edited where needed to make CI happy (think portability issues of paths).
That means that these errors have not been checked for correctness, they were previously ignored and some of them are likely silently faulty.
I think we should all go through them and make notes of problematic looking errors.
In places where no consistent pattern for the errors could be found, we still allow
The
----with no further message makes this explicit, rather than the implicit version we used to have for all of these where the trailing----was left off entirely.Some examples of places where this happens is with heavy use of
foreachandloopvariables.Ideally we tear these apart if the error messages diverge drastically, so we don't have to resort to silently ignoring the actual error produced.