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.

Sign up for GitHub

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

Jump to bottom

Fix/qgstest improv #58212

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix/qgstest improv #58212

wants to merge 6 commits into from

Conversation

benoitdm-oslandia
Copy link
Contributor

Add a fix and a feature to qgstest:

  • add a check if the image if null in checkimage
  • add new verify against long string/bytearray

ptitjano and others added 2 commits July 23, 2024 08:33
@ptitjano @benoitdm-oslandia
qgstest: Check image is not null
7816523
@benoitdm-oslandia
qgstest: add new check to compare long QString/QByteArray with an exp…
6649af6 
…ected file content.

This check allows to read expected data from file and to compare the actual and expected content chunk by chunk. This chunk by chunk is need as for long QString or QByteArray, only the start of strings are displayed when error occurs.
Hexadecimal and string content are displayed when a diff occurs.
@github-actions github-actions bot added this to the 3.40.0 milestone Jul 23, 2024
@benoitdm-oslandia benoitdm-oslandia self-assigned this Jul 23, 2024
@benoitdm-oslandia benoitdm-oslandia added the testsuite Issue related to testsuite label Jul 23, 2024
nyalldawson
nyalldawson reviewed Jul 23, 2024
View reviewed changes
src/test/qgstest.h Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 23, 2024 •
edited
Loading

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit ce9b9ac)

lbartoletti
lbartoletti reviewed Jul 24, 2024
View reviewed changes
src/test/qgstest.h Outdated Show resolved Hide resolved
src/test/qgstest.h Outdated Show resolved Hide resolved
src/test/qgstest.h Show resolved Hide resolved
src/test/qgstest.h Outdated Show resolved Hide resolved
@benoitdm-oslandia
Copy link
Contributor Author

FYI the output in case of failure is something like that:

309: QWARN  : TestQgs3DRendering::testAmbientOcclusion() Hex version of the parts of array that differ starting from char  0 (actual vs expected):  
309:  "285174334452656e6465723a3a5152656e6465725375726661636553656c6563746f727b372f3c6e6f5f6e616d653e7d290a2020285174334452656e6465723a3a5156696577706f72747b382f3c6e6f5f6e616d653e7d290a2020202028517433445265" 
309:  "3c3c3c3c3c3c3c20484541440a285174334452656e6465723a3a5152656e6465725375726661636553656c6563746f727b382f3c6e6f5f6e616d653e7d290a2020285174334452656e6465723a3a5156696577706f72747b392f3c6e6f5f6e616d653e7d"
309: FAIL!  : TestQgs3DRendering::testAmbientOcclusion() Comparison failed in function 'testAmbientOcclusion' starting from char 0.
309:    Actual   (/tmp/actual_ambient_occlusion_2_framegraph.txt)                        : (Qt3DRender::QRenderSurfaceSelector{7/<no_name>})
309:   (Qt3DRender::QViewport{8/<no_name>})
309:     (Qt3DRe
309:    Expected (control_files/3d/expected_ambient_occlusion_2/expected_framegraph.txt): <<<<<<< HEAD
309: (Qt3DRender::QRenderSurfaceSelector{8/<no_name>})
309:   (Qt3DRender::QViewport{9/<no_name>}]
309:    Loc: [/home/user/qgis/tests/src/3d/testqgs3drendering.cpp(1587)]
309: FAIL!  : TestQgs3DRendering::testAmbientOcclusion() 'checkLongStr(__FILE__, __FUNCTION__, __LINE__, "ambient_occlusion_2", "framegraph.txt", actualFG.toUtf8())' returned FALSE. ()
309:    Loc: [/home/user/qgis/tests/src/3d/testqgs3drendering.cpp(1587)]

@benoitdm-oslandia
Copy link
Contributor Author

@lbartoletti @nyalldawson ready for review!

I had tested the test function with a small test, I am wondering if I should create and keep a unittest to test this new function?

nyalldawson
nyalldawson reviewed Jul 25, 2024
View reviewed changes
src/test/qgstest.h

if ( actualStr.size() != expectedStr.size() )
{
qWarning() << "Array have not the same length (actual vs expected):" << actualStr.size() << "vs" << expectedStr.size();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this fail? Eg if the actual string is a truncated version of expected, then the test will currently pass...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what I have tested, this works fine but maybe I did not understand what you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean that if something regresses and starts giving a shorter string, this test will warm but still pass. It should definitely fail if the strings are different lengths...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, the test will fail just after in the for loop when it will compare portions of the expected and the actual. Even if one of the actual or expected string are empty.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but you're only looping through up to strSize, which is defined as actualStr.size(). So if the expectedStr.size() is greater than the actualStr.size(), you'll never compare those missing bits of actualStr to what they should be.

I think line 319 should be const int strSize = std::max( actualStr.size(), expectedStr.size() );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyalldawson nyalldawson nyalldawson left review comments

@lbartoletti lbartoletti lbartoletti left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@benoitdm-oslandia benoitdm-oslandia

Labels
testsuite Issue related to testsuite
Projects
None yet
Milestone
3.40.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@benoitdm-oslandia @nyalldawson @lbartoletti @ptitjano