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

Test for bundled libraries like libjpeg,libpng, ICU #1036

Open
wants to merge 25 commits into
base: qtwebkit-5.212
Choose a base branch
from

Conversation

RAJAGOPALAN-GANGADHARAN
Copy link
Collaborator

No description provided.

@@ -59,7 +60,7 @@ foreach (testName ${QtWK1ApiTests})
add_executable(tst_${testName} ${tst_${testName}_SOURCES})
target_include_directories(tst_${testName} PRIVATE ${testName})
target_link_libraries(tst_${testName} ${QtWK1ApiTests_LIBRARIES})
set_target_properties(tst_${testName} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${QtWK1ApiTests_RUNTIME_OUTPUT_DIRECTORY} AUTOMOC ON)
set_target_properties(tst_${testName} PROPERTIES RUNTIME_OUTPUT_DIRECTORY_${CMAKE_BUILD_TYPE_UPPER} ${QtWK1ApiTests_RUNTIME_OUTPUT_DIRECTORY} AUTOMOC ON)
Copy link
Member

Choose a reason for hiding this comment

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

Some CMake generators (e.g. Visual Studio, Ninja Multi-Config) support multiple build types in one build, so this way is not correct. I think we need something like

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}")
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}")

like e.g. in OptionsWin.cmake

{
QString html("<html>"
"<body>"
"<img src='qrc:///image.jpg'>"
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK <html> and <body> will be added automatically if you just use "<img src='qrc:///image.jpg'>"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the browser itself adds those tags by default not only for qrc. So yeah I will fix this.


void tst_Libjpeg::decodeAndCompare()
{
QString html("<html>"
Copy link
Member

Choose a reason for hiding this comment

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

QStringLiteral

@annulen
Copy link
Member

annulen commented Oct 18, 2020

Looks good! Now you can proceed with other tests


void tst_Libjpeg::cleanupTestCase()
{
}
Copy link
Member

Choose a reason for hiding this comment

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

No need to have empty constructors, init/cleanup

QVariant res = page.mainFrame()->evaluateJavaScript("res");
QCOMPARE(res.toString(), QStringLiteral("Database Success"));

page.mainFrame()->evaluateJavaScript("add('1','Name1')");
Copy link
Member

Choose a reason for hiding this comment

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

Missing space afrer ","

@annulen
Copy link
Member

annulen commented Nov 16, 2020

  1. png is added to cmake project but missing in PR
  2. tests/3rdpartylibs/CMakeLists.txt is almost exact copy of tests/webkitwidgets/CMakeLists.txt, it is not acceptable
  3. Branch is not rebased

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.

None yet

2 participants