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

Sandraros/check demo regressions 2 #940

Merged
merged 9 commits into from Jan 5, 2022

Conversation

sandraros
Copy link
Collaborator

@sandraros sandraros commented Dec 31, 2021

Fix #894, alternative to #902.

This is a lightweight solution, the demo programs are not changed except those ones which write sy-datum and sy-uzeit (EDIT: and demo programs which read database tables to produce Excel content → fixed data provided).

Copy link
Member

@AndreaBorgia-Abo AndreaBorgia-Abo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patience with my review speed! Here are my observations:

  1. not requiring an API of sorts to write a test report is arguably a pro, since users might want to adapt a test report when starting out with this library
  2. we still need to account for date and time, as already noted elsewhere, but even this might not be enough: for example, in ZEXCEL_DEMO3 data coming from table SFLIGHT is different between NPL and A4H demo systems. I don't have a suggested fix here: ATDF is not supported on all versions, rewriting the tests is perhaps not worth the effort anyway since developers are the target users here... I guess we'll simply have to document it somewhere in the program itself or via a popup
  3. going through the GUI probably restricts the demo checker to the Windows version: whereas the checker from the older PR would run just fine with the Java GUI, this one has the typical UNKNOWN_DP_ERROR control issue (this time in ZEXCEL_DEMO28)
  4. in A4H (for this PR) all Reader tests fail because of a spurious tag: ht="0.0000000000000000E+00"
  5. in NPL (for the older, as reference only) many tests fail because of this extra tag: width="9.10" (apparently coming from CREATE_XL_SHEET but I suspect that's because of the iterator changes which were made afterwards)
  6. if the output directory already contains the test files, the checker cannot run; I think we could either create a temporary directory for each run or just allow overwriting the files, perhaps informing the user once

All in all, this is a better solution and you may un-draft this PR.
With regards to the older one, I think we could close it without merging and perhaps conserve the corresponding tree as food for thought, if anything else.

@sandraros sandraros marked this pull request as ready for review January 3, 2022 17:49
@sandraros
Copy link
Collaborator Author

sandraros commented Jan 3, 2022

Thanks to you! :)

  1. So, closing the other solution (Sandraros/check demo regressions #902) and keeping the branch for a few days, in case...
  2. I'd propose to have a hidden parameter in ZDEMO_EXCEL3 to generate hardcoded lines. I will push something.
  3. Difficult to solve UNKNOWN_DP_ERROR in ZDEMO_EXCEL28/Java GUI.
  4. I created Writer: useless null row height (ht="0.0000000000000000E+00") #944. I'll propose a PR quickly.
  5. The iterator fix has added width="9.10" for all columns and the Excel files in the Web repository should reflect that, so I wonder if you have pressed the Refresh button to call ZDEMO_EXCEL/regenerate the demo files in the output directory.
  6. Why do you say "if the output directory already contains the test files, the checker cannot run"? I don't have any issue.

@AndreaBorgia-Abo
Copy link
Member

  1. not sure if this is the only report affected but we'll soon find out :)
  2. I know, but this solution is so much better that I'm willing to live with it
  3. regeneration should not be needed: I wasn't jumping from one branch to the other, rather I had pulled the older PR into NPL and the newer in A4H so comparing the behaviour was a simple matter of switching window
  4. because I misinterpreted the error messsage, that's why: it only happens if you leave Excel open on any of the test files, in which case it will just dump

Copy link
Member

@AndreaBorgia-Abo AndreaBorgia-Abo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there but not quite... and the test results are weird!

In A4H only:

  • ZDEMO_EXCEL3
  • ZDEMO_EXCEL10
  • ZDEMO_EXCEL22 (also: top 3 rows are frozen, instead of just one, perhaps something was removed above)
  • ZDEMO_EXCEL33

Both in NPL and in A4H:

This time I pulled the same branch in both systems, of course. Tomorrow I'll look into the A4H issues, unless you already have an idea.

@sandraros
Copy link
Collaborator Author

sandraros commented Jan 4, 2022

In A4H only: all these demo programs get data from tables SFLIGHT or T005T. Let me provide same solution as I did for ZDEMO_EXCEL3 (static data not from tables).

EDIT: fixed.

@AndreaBorgia-Abo
Copy link
Member

Adding suggestion here, as already noted it's not possible to suggest changes elsewhere:

columns->get_column( 'COMPARE_XLSX_JUST_NOW' )->set_technical( ).
columns->get_column( 'COMPARE_XLSX_REFERENCE' )->set_technical( ).

@sandraros
Copy link
Collaborator Author

sandraros commented Jan 5, 2022

Adding suggestion here, as already noted it's not possible to suggest changes elsewhere

It's brand new program, all lines are new, are you sure it isn't possible to suggest changes?

Co-authored-by: Abo <andrea@borgia.bo.it>
Copy link
Member

@AndreaBorgia-Abo AndreaBorgia-Abo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's brand new program, all lines are new, are you sure it isn't possible to suggest changes?

I did not see it in the changes, perhaps because it was older. Found it now and added, thanks. Oh, another suggestion: a call transaction SMW0 to download the existing reference file, a companion function to the save button.

Anyhow, there are still some differences reported in A4H:

  • ZEXCEL_DEMO3: sort order of the second sheet (Data validation) is different, but all values are present
  • ZEXCEL_DEMO33: as far as I can tell the visible content is unchanged but internally some nodes still have different numbering(?)

I think we could merge this and then later understand where those differences come from.

sheet33-postPR-1

sheet33-postPR-2

sheet33-postPR-3

@sandraros
Copy link
Collaborator Author

I understand, it's the column headings from SFLIGHT/T005T data elements, they may differ in each SAP version, we should fix them also.

@sandraros
Copy link
Collaborator Author

Agreed to merge now if you wish. I'll work on the fix this evening.

@AndreaBorgia-Abo AndreaBorgia-Abo merged commit 3ef605e into master Jan 5, 2022
@AndreaBorgia-Abo AndreaBorgia-Abo deleted the sandraros/check-demo-regressions-2 branch January 5, 2022 09:31
@AndreaBorgia-Abo
Copy link
Member

Agreed to merge now if you wish. I'll work on the fix this evening.

Regarding the remaining differences, would you like a single ticket for both NPL and A4H or would you rather keep them distinct?

@sandraros
Copy link
Collaborator Author

Not sure what you mean. I think I'll do one PR (didn't have free time yesterday) to make NPL and A4H produce the same result. In ZDEMO_EXCEL3 I forgot to fix table data instead of SELECT * FROM SCARR, and in ZDEMO_EXCEL33 I think we need to fix the line of column headings with set_cell_value( ... ) right after calling the convert method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit Tests to avoid regression in existing demo programs
2 participants