assertCSVDataSet fails when csv dataset to assert starts with all zeros

[Wipster]

What are you trying to achieve?

Successfully run the assertCSVDataSet function on data that was inserted into the db.

What do you get instead?

assertCSVDataSet fails on running the Test with the message "Not asserted record found for ..."

How to reproduce the issue?

1. Create csv file containing data that should be present in the db at the point of assertion.
2. Start the lines with a flag you expect to be 0 e.g.:

"my_ext_table",,,
,"deleted","description","some_field"
,0,"Foo","A"
,0,"Bar","B"
,0,"Test","C"

3. Rearranging the column order leads to the function to assert correctly again:

"my_ext_table",,,
,"description","some_field","deleted"
,"Foo","A",0
,"Bar","B",0
,"Test","C",0

Specify some data of the environment

• TYPO3 testing framework version: 6.16.7
• TYPO3 version: 11.5.25
• TYPO3 installation type: git + composer
• PHP version: 8.1
• Web server used + version: ddev with nginx
• Operating system: MacOS

---

Related to #488

[Wipster]

Looks like the problem is here Classes/Core/Functional/Framework/DataHandling/DataSet.php line 108

} elseif ($tableName !== null && !empty($values[1])) {

I would love to provide a pull request with a fix, but I'm not sure why this check is there in the first place. !empty($values[1]) could e.g. be replaced with a check for an empty string.

[sbuerk]

I guess the check is there, as the first value/column should be the primery key value/autoincrement value of the table (and therefore uid in TYPO3 world. Without
that, an assertment can easyly go wrong as it matches more records than only one specific.

So simply removing that empty check is a quick workaround, but not really reliable - and leading to false checks quite quickly.

So you may implement ann assert then more on yourself with a query:

select count(*) as cnt WHERE field1 = 1 AND .... HAVING cnt > 0

Would vote against removing that check. Eventually it should be more combined to check for the real PK/field etc ... (which is quite a bigger change than).

[Wipster]

@sbuerk Maybe an exception would be nice for the user to see why the test is failing. Currently it's kind of failing "silently" giving the message Not asserted record found for ... although you see the record and the fields are all matching.

[sbuerk]

A related second issue has reported with #488.

[alexanderschnitzler]

I guess the check is there, as the first value/column should be the primery key value/autoincrement value of the table (and therefore uid in TYPO3 world. Without
that, an assertment can easyly go wrong as it matches more records than only one specific.

I don't follow that argument because

• There are columns without uid (ManyToMany)
• There are cases where I don't want/need to provide the uid because it's auto incrementing.

Let's please fix this and allow "0" as value.

Addendum:

From an architectural standpoint a parser is not the place to check data integrity. It should care about syntax, not semantics. The latter can be checked with post parse validation (if necessary). A parser that drops syntactically correct data is not doing a good job.

[sbuerk]

Ack. (And was already ack, just not commented it). Will look into it today, I guess.

[alexanderschnitzler]

Thanks a lot!

[lolli42]

I don't agree:

In functional tests we do and should know the result uid's since the entire scenario is under functional test control. Skipping the auto increment is not a good idea and makes asserting those harder than they should be.

Note asserting non-uid tables works well: Core has many scenarios with sys_refindex (where primary key is a hash), and also with MM rows where primary key is a combination of multiple fields.

[Wipster]

@lolli42 You woudn't loose the option to specify uids, but given the possibility to skip them which would be a nice time-saver. I don't see any reason to force people to know their uids in a sut.

[alexanderschnitzler]

@lolli42 Ok, so what I get from the argumentation is:

• Code is fine
• You're doing it wrong
• Do it right and you're happy

A parser that silently fails on syntactically correct data seems to be just alright.

One more patch to my list of composer patches for each project then.

[oliverklee]

Everybody, please let's revisit this.

Independent of how much sense the use cases makes, the current code is incorrect (related to what it should do). It should only catch empty string, but not falsey vales.

[lolli42]

...

I'll rediscuss with stefan. One reason is that having the uid in assert-csv makes it very easy for the asserter to locate the rows and to sort out if all rows have been handled, which is usually what we want: There shouldn't be neither more, nor less rows than what is stated in the assert-csv. That's the deal. Ditching the uid restriction makes it potentially harder and will need a more serious look at the asserter.

[oliverklee]

I'd suggest to keep the "is there a UID?" check, but change it from empty to === ''.

[lolli42]

This is solved with #488 being done, is it?

[sbuerk]

Solved with #488, confirmed by opener.