Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

warning.table.characters_between_fields missing for last record in table #431

Closed
jordanpadams opened this issue Oct 26, 2021 · 4 comments
Closed

Comments

@jordanpadams
Copy link
Member

πŸ› Describe the bug identified during I&T

Report by @rchenatjpl, failed test

πŸ₯Ό Related Test Case(s)

πŸ” : Related issues

πŸ¦„ #57


βž• Additional Details

πŸ“œ To Reproduce

Steps to reproduce the behavior see:

πŸ•΅οΈ Expected behavior

it would throw a warning for line 4 of this table:

2019-08-06T00:00:00Z This is a test 1111 2222 3333 4444
2019-08-06T00:01:00Z,This is a test q1111 2222 3333 4444
2019-08-06T00:02:00Z This is a test 1111 2222 3333 4444
2019-08-06T00:03:00ZaThis is a test b1111c2222d3333e4444

πŸ“š Version of Software Used

Version 2.2.0-SNAPSHOT
Date 2021-10-26T05:35:59Z

🩺 Test Data / Additional context

See comments above to create new test data.


πŸ¦„ Related requirements

βš™οΈ Engineering Details

@al-niessner
Copy link
Contributor

@jordanpadams @rchenatjpl

Is this a bug or is the code behaving as desired? I modified the table to look like this:

2019-08-06T00:00:00Z This is a test    1111 2222 3333 4444
2019-08-06T00:01:00Z This is a test    1111 2222 3333 4444
2019-08-06T00:02:00Z This is a test    1111 2222 3333 4444
2019-08-06T00:03:00ZaThis is a test   b1111c2222d3333e4444

and validate correctly identifies the characters in the column:

  PASS: file:/home/niessner/Projects/PDS/validate/src/test/resources/github57/validate_57c.xml
      WARNING  [warning.table.characters_between_fields]   Unexpected alphanumeric characters found between fields in record 4:  [a] found between fields 1 and 2
      WARNING  [warning.table.characters_between_fields]   Unexpected alphanumeric characters found between fields in record 4:  [b] found between fields 2 and 3
      WARNING  [warning.table.characters_between_fields]   Unexpected alphanumeric characters found between fields in record 4:  [c] found between fields 3 and 4
      WARNING  [warning.table.characters_between_fields]   Unexpected alphanumeric characters found between fields in record 4:  [d] found between fields 4 and 5
      WARNING  [warning.table.characters_between_fields]   Unexpected alphanumeric characters found between fields in record 4:  [e] found between fields 5 and 6
        1 product validation(s) completed

The reason the errors are not reported with the table supplied is the preceding errors on row 2 suppress the later ones on row 5 from this bit of code:

// If any of the columns had reported a warning/error, set the reportedErrorFlag
// to true
// so as not to overwhelm the error reporting mechanism.
for (int ii = 0; ii < fieldNumberList.size(); ii++) {
if (columnsWarningFlagList.get(ii) == Boolean.TRUE) {
this.reportedErrorFlag = true;
}
}

If we only want the first row with column errors, as the code suggests, then this is false positive error. If we want to show all random warning.table.characters_between_fields, then an earlier part of the code will need to be changed. Please clarify the desired behavior.

@rchenatjpl
Copy link

Hmm, tough call. In general I'd like to see all errors/warnings called out with a max of maybe 5 of the same type of error. I think it's acceptable that only the first error gets called out, but I can imagine that a data file would deliberately repeat such an error (Mark Bentley's original example had a data file that was trying to be both fixed-width and comma-separated) and tolerate warnings about that error AND have other bugs that would need attention. Sorry, this may all be no help.

@jordanpadams
Copy link
Member Author

@al-niessner I think this is functioning as expected. the original ticket I think was not checking the last record in the table. so I think this has already been fixed.

per @rchenatjpl's comment, I think that is maybe something we can add a new requirement where we can maybe allow users to disable WARNINGs as the would like. although, in the end, the summary does say all the warnings are of 1 type, so they can always just ignore those if they know those are not an issue.

@jordanpadams
Copy link
Member Author

closed per #642

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: 🏁 Done
Development

No branches or pull requests

5 participants