adding windows test driver fix #293

kreuzberger · 2024-04-08T08:55:37Z

Summary

Details

Different QtTestDriver Implemenation to ensure no resource lock on windows.
Issue with undefined output format for qExec, add explicitly ".txt" extension

Motivation and Context

Fix #292

How Has This Been Tested?

Running internal tests with/without changes on ubuntu 22.04 and win10 vs2019

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

It is my own work, its copyright is implicitly assigned to the project and no substantial part of it has been copied from other sources (including Stack Overflow). In rare occasions this is acceptable, like in CMake modules where the original copyright information should be kept.
I'm using the same code standards as the existing code (indentation, spacing, variable naming, ...).
I've added tests for my code.
I have verified whether my change requires changes to the documentation
My change either requires no documentation change or I've updated the documentation accordingly.
My branch has been rebased to main, keeping only relevant commits.

ursfassler

Thanks for the contribution!

I see the problem and how you solve it. Please fix and adjust your changes according my input (or discuss it when you see it different 🙂 ).

Please also add the test in the CI. This helps me to see the problem and protects us from similar problems in the future.

src/drivers/QtTestDriver.cpp

kreuzberger · 2024-04-10T19:47:09Z

Due to the checks: Could you provide a clang-format file (.clang-format) or what should be used for clang-format as default style? Assume clang-format without any further options?

ursfassler · 2024-04-11T06:20:13Z

Due to the checks: Could you provide a clang-format file (.clang-format) or what should be used for clang-format as default style? Assume clang-format without any further options?

The format file is checked in: https://github.com/cucumber/cucumber-cpp/blob/main/.clang-format
Clang-format may produce slightly different out depending on the version. It usually is no problem.

kreuzberger · 2024-04-11T14:39:47Z

Current Status

TLDR: Windows Tests integration, would only merge after some clarifications

Open Issues

On Windows the QtTestDriver failes sometimes. Reason is that in one test the Text Error messages are compared.
Due to the nature of the default QtTest output the diff test fails cause the Test execution time differs.
This could also occur on linux, but seems not so often there (currently im running windows in VM)

+ Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 2ms
+ ********* Finished testing of cucumber::internal::QtTestObject *********
- Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 1ms
- ********* Finished testing of cucumber::internal::QtTestObject *********

Dont know how to fix.

For the QtTests to work which are not part of ctest the Qt Runtime Path (bin on Windows) must be in PATH.
Do currently not know HOW to get this in the CI. Workarounded by having it in the PATH env of the caller.
This could be fixed running these Tests in CMake with the runtime Path determined like for the other ctests and let the test code (starting and waiting for the processes) be run with os dependent scripts.

…iable

kreuzberger · 2024-04-15T18:14:46Z

Hi! Would it be possible to let the checks run on commit, so i can see open issues and fix them?

ursfassler

I looked through the changes shortly to see if I can run the tests, i.e. that no bad things are in there 😄

I haven't really understood the problem with Qt, Windows and tests, hence I have troubles to understand if the patch is good or not. Sorry for that, a bit short on time on my end.

run-linux.sh

run-windows.ps1

src/drivers/QtTestDriver.cpp

tests/CMakeLists.txt

ursfassler · 2024-04-15T19:21:14Z

On Windows the QtTestDriver failes sometimes. Reason is that in one test the Text Error messages are compared.
Due to the nature of the default QtTest output the diff test fails cause the Test execution time differs.

😲 That would be a bad tests, timings shouldn't be compared in the tests. What test is it?

ursfassler · 2024-04-15T19:22:38Z

Fixed run-linux.sh issue to use Qt6 and disable Qt5 if both are found

Idea is that only the Qt version you specified is searched for.

kreuzberger · 2024-04-15T20:04:06Z

On Windows the QtTestDriver failes sometimes. Reason is that in one test the Text Error messages are compared.
Due to the nature of the default QtTest output the diff test fails cause the Test execution time differs.

😲 That would be a bad tests, timings shouldn't be compared in the tests. What test is it?

QtTestDriverTest, see open issues from above

ursfassler · 2024-04-29T12:24:43Z

@kreuzberger please see my PR #296 to fix the flaky test.

I also investigated if it is possible to capture the test log without needing the file. Unfortunately it seems not to be possible (without too much hacking). Meaning that this PR is quite relevant.

kreuzberger · 2024-05-02T14:18:30Z

@kreuzberger please see my PR #296 to fix the flaky test.

I also investigated if it is possible to capture the test log without needing the file. Unfortunately it seems not to be possible (without too much hacking). Meaning that this PR is quite relevant.

Commented your PR #296.

ursfassler · 2024-05-09T07:35:45Z

@kreuzberger unfortunetaly the Windows build has a problem with gtest.

@cwellm did you also had this problem? Could you also help out with reviewing the windows code?

ursfassler

@kreuzberger I think we get closer on your fix but still have troubles with the CI. I'd suggest that you split your changes to a small fix for the temporary file on Windows and we add the tests in a separate PR. This way you have the changes that you need integrated.

If you agree, I'd suggest that you open a second PR with the Windows-tests and only keep the functional changes here. Please also cleanup the history in order to only have relevant commits in it.

CMakeLists.txt

ursfassler · 2024-05-09T07:44:05Z

src/drivers/QtTestDriver.cpp

+// wraps the QTemporaryFile creation
+// on Windows the file could not be written as long as QTemporaryFile owner of the file.


Spelling, I'd suggest (but I'm not a native speaker):

// Wraps the QTemporaryFile creation. // Needed because on Windows, the file can not be written from QtTest as long as we keep it open.

src/drivers/QtTestDriver.cpp

ursfassler · 2024-05-09T07:58:39Z

src/drivers/QtTestDriver.cpp

+    const auto file = TemporaryFileWrapper::create();
+    if (!file.exists()) {
        return InvokeResult::failure("Unable to open temporary file needed for this test");
    }
-    file.close();

    QtTestObject testObject(this);
    int returnValue = QTest::qExec(
        &testObject,
        QStringList() << "test"
-                      << "-o" << file.fileName()
+                      << "-o" << file.name()
    );
+
    if (returnValue == 0) {
        return InvokeResult::success();
    } else {
-        file.open();
-        QTextStream ts(&file);
-        return InvokeResult::failure(ts.readAll().toLocal8Bit());
+        return InvokeResult::failure(file.read().toLocal8Bit());


Looks good 👍

ursfassler · 2024-05-09T08:00:42Z

tests/CMakeLists.txt

+function(cuke_set_environment environment)
+    set(options)
+    set(oneValueArgs )
+    set(multiValueArgs RUNPATH)
+
+    cmake_parse_arguments(CUKE_ENV "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
+
+    if(NOT "${CUKE_ENV_UNPARSED_ARGUMENTS}" STREQUAL "")
+        message(
+            FATAL_ERROR
+                "unparsed arguments in call to cuke_set_environment: ${CUKE_ENV_UNPARSED_ARGUMENTS} from ${CMAKE_CURRENT_LIST_FILE}"
+        )
+    endif()
+
+    if( NOT "${CUKE_ENV_RUNPATH}" STREQUAL "")
+        if(WIN32)
+            string(REPLACE ";" "\;" CUKE_ENV_RUNPATH "${CUKE_ENV_RUNPATH}")
+        endif()
+        set(RUNPATH "$<IF:$<PLATFORM_ID:Windows>,PATH,LD_LIBRARY_PATH>")
+        list(APPEND environment "$<IF:$<PLATFORM_ID:Windows>,PATH,LD_LIBRARY_PATH>=path_list_prepend:$<SHELL_PATH:${CUKE_ENV_RUNPATH}>")
+    endif()
+    set(${environment} ${${environment}} PARENT_SCOPE)
+endfunction()
+


This is a lot of code to just support Windows. Is it possible to move this to run-windows.ps1 (if it makes sense).

@cwellm any ideas from your side?

src/drivers/QtTestDriver.cpp

adding windows test driver fix

83c7559

ursfassler requested changes Apr 10, 2024

View reviewed changes

kreuzberger added 3 commits April 11, 2024 09:04

integrate changes from review

8ec2b38

run more windows tests

75e4b0f

fix linux tests (both qt5 and qt6 available)

cfd1cd4

kreuzberger requested a review from ursfassler April 11, 2024 14:48

kreuzberger added 7 commits April 12, 2024 08:00

determine cuke_qt runtime path for qt tests from library names as var…

6895dc0

…iable

fix CUKE_RUNTIME_PATH scope issue

dc83164

integrate QTemporaryTestDriver suggestion

b9232fd

fix clang-format of driver

bcb880d

fix linux compilation errors

aa604ef

add pid and app name to temp file template

85903d2

clang-format changed QtTestDriver

6a18103

ursfassler reviewed Apr 15, 2024

View reviewed changes

applied changes required from review

3ffd0b6

kreuzberger requested a review from ursfassler April 26, 2024 06:19

ursfassler mentioned this pull request Apr 29, 2024

fix flaky QtTestDriver test #296

Merged

9 tasks

ursfassler reviewed May 9, 2024

View reviewed changes

src/drivers/QtTestDriver.cpp Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding windows test driver fix #293

adding windows test driver fix #293

kreuzberger commented Apr 8, 2024 •

edited

ursfassler left a comment

kreuzberger commented Apr 10, 2024 •

edited

ursfassler commented Apr 11, 2024

kreuzberger commented Apr 11, 2024 •

edited

kreuzberger commented Apr 15, 2024

ursfassler left a comment

ursfassler commented Apr 15, 2024

ursfassler commented Apr 15, 2024

kreuzberger commented Apr 15, 2024 •

edited

ursfassler commented Apr 29, 2024

kreuzberger commented May 2, 2024 •

edited

ursfassler commented May 9, 2024

ursfassler left a comment

ursfassler May 9, 2024

ursfassler May 9, 2024

ursfassler May 9, 2024

		// wraps the QTemporaryFile creation
		// on Windows the file could not be written as long as QTemporaryFile owner of the file.

adding windows test driver fix #293

Are you sure you want to change the base?

adding windows test driver fix #293

Conversation

kreuzberger commented Apr 8, 2024 • edited

Summary

Details

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

ursfassler left a comment

Choose a reason for hiding this comment

kreuzberger commented Apr 10, 2024 • edited

ursfassler commented Apr 11, 2024

kreuzberger commented Apr 11, 2024 • edited

Current Status

Open Issues

kreuzberger commented Apr 15, 2024

ursfassler left a comment

Choose a reason for hiding this comment

ursfassler commented Apr 15, 2024

ursfassler commented Apr 15, 2024

kreuzberger commented Apr 15, 2024 • edited

ursfassler commented Apr 29, 2024

kreuzberger commented May 2, 2024 • edited

ursfassler commented May 9, 2024

ursfassler left a comment

Choose a reason for hiding this comment

ursfassler May 9, 2024

Choose a reason for hiding this comment

ursfassler May 9, 2024

Choose a reason for hiding this comment

ursfassler May 9, 2024

Choose a reason for hiding this comment

kreuzberger commented Apr 8, 2024 •

edited

kreuzberger commented Apr 10, 2024 •

edited

kreuzberger commented Apr 11, 2024 •

edited

kreuzberger commented Apr 15, 2024 •

edited

kreuzberger commented May 2, 2024 •

edited