On Sun, Nov 27, 2022 at 07:04:46PM +0800, Julien Rouhaud wrote:
> And here's the rebased patch for the TAP tests. I will switch the CF entry to
> Needs Review.
I have been looking at that, and applied the part 1 of the test for
the positive tests to cover the basic ground I'd like to see covered
for this feature. I have done quite a few changes to this part, like
reworking the way the incrementation of the counters is done for the
rule/map numbers and the line numbers of each file. The idea to use a
global dictionary to track the current number of lines was rather
clear, so I have kept that. add_{hba,ident}_line() rely on the
files included in a directory to be ordered in the same way as the
backend, and I am fine to live with that.
+# Normalize the data directory for Windows
+$data_dir =~ s/\/\.\//\//g; # reduce /./ to /
+$data_dir =~ s/\/\//\//g; # reduce // to /
+$data_dir =~ s/\/$//; # remove trailing /
+$data_dir =~ s/\\/\//g; # change \ to /
The mysticism around the conversion of the data directory path is
something I'd like to avoid, or I suspect that we are going to have a
hard time debugging any issues in this area. For the positive cases
committed, I have gone through the approach of using the base name of
the configuration file to avoid those compatibility issues. Wouldn't
it better to do the same for the expected error messages, switching to
an approach where we only check for the configuration file name in all
these regexps?
+# Generate the expected output for the auth file view error reporting (file
+# name, file line, error), for the given array of error conditions, as
+# generated generated by add_hba_line/add_ident_line with an err_str.
+sub generate_log_err_rows
+{
generate_log_err_rows() does not strike me as a good interface. The
issues I am pointed at here is it depends directly on the result
returned by add_hba_line() or add_ident_line() when the caller passes
down an error string. As far as I get it, this interface is done
as-is to correctly increment the line number of one file, and I'd like
to believe that add_{hba,ident}_line() should be kept designed so as
they only return the expected matching entry in their catalog views,
without any code path returning something else (in your case the
triplet [ $filename, $fileline, $err_str ]). That makes the whole
harder to understand.
Attached is a rebased patch of the rest. With everything we have
dealt with in this CF, perhaps it would be better to mark this entry
as committed and switch to a new thread where the negative TAP tests
could be discussed? It looks like the part for the error pattern
checks compared with the logs could be the easiest portion of what's
remaining.
--
Michael
