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

[SHELL32_APITESTS] Add ROS.ico test file. Improve shell32:ExtractIconEx tests #5169

Merged
merged 6 commits into from
Mar 30, 2023
Merged

[SHELL32_APITESTS] Add ROS.ico test file. Improve shell32:ExtractIconEx tests #5169

merged 6 commits into from
Mar 30, 2023

Conversation

Doug-Lyons
Copy link
Contributor

@Doug-Lyons Doug-Lyons commented Mar 18, 2023 •
edited

Purpose

Improve shell32:ExtractIconEx tests by updating it and adding more test files and 1 new icon file.

JIRA issue: ROSTESTS-381

Proposed changes

Add new ROS.ico file which has both a normal icon and a PNG one.
Update testing to test for count of icons in file and separately extract the 0th one.
Add tests for sysicon.ico, explorer.exe and the new ROS.ico files.

WinWP:
WinXP_ExtractIconEx

W2K3SP2:
W2K3SP2_ExtractIconEx

@github-actions github-actions bot added the ROSTESTS Label for ROS testcases PRs. label Mar 18, 2023
@Doug-Lyons Doug-Lyons force-pushed the shell32_apitest_ExtractIconEx_additions branch 2 times, most recently from 9b64e26 to ab085b7 Compare March 21, 2023 22:20
@Doug-Lyons Doug-Lyons force-pushed the shell32_apitest_ExtractIconEx_additions branch from 459f642 to 525098d Compare March 21, 2023 22:29
@Doug-Lyons Doug-Lyons requested review from HBelusca and katahiromz and removed request for HBelusca and katahiromz March 21, 2023 22:32
@Doug-Lyons
Copy link
Contributor Author

For some reason, github will not allow me to request reviews from both @katahiromz and @HBelusca without deleting the review request from the other. Unless there is something that I am doing wrong. Sorry for the confusion. I am open to suggestions. Thanks.

Doug-Lyons
Doug-Lyons commented Mar 21, 2023
View reviewed changes
Copy link
Contributor Author

@Doug-Lyons Doug-Lyons left a comment

Choose a reason for hiding this comment

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

I have some "pending" items and hopefully this will fix them.

modules/rostests/apitests/shell32/ExtractIconEx.cpp Outdated Show resolved Hide resolved
modules/rostests/apitests/shell32/ExtractIconEx.cpp Outdated Show resolved Hide resolved
@Doug-Lyons Doug-Lyons force-pushed the shell32_apitest_ExtractIconEx_additions branch from a043060 to 6ff16e2 Compare March 22, 2023 02:18
@Doug-Lyons Doug-Lyons force-pushed the shell32_apitest_ExtractIconEx_additions branch from d8e4135 to a44060d Compare March 22, 2023 02:23
@Doug-Lyons Doug-Lyons requested review from katahiromz and removed request for HBelusca March 22, 2023 03:49
katahiromz
katahiromz approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@katahiromz katahiromz 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 go.

@JoachimHenze JoachimHenze changed the title [SHELL32_APITESTS] Add ROS.ico test file. Improve shell32:ExtractconEx tests [SHELL32_APITESTS] Add ROS.ico test file. Improve shell32:ExtracticonEx tests Mar 22, 2023
@JoachimHenze JoachimHenze changed the title [SHELL32_APITESTS] Add ROS.ico test file. Improve shell32:ExtracticonEx tests [SHELL32_APITESTS] Add ROS.ico test file. Improve shell32:ExtractIconEx tests Mar 22, 2023
@JoachimHenze
Copy link
Contributor

Ftr, not mentioned in the description but there were even failing tests before this PR in that test:
https://reactos.org/testman/compare.php?ids=86524 VBox
https://reactos.org/testman/compare.php?ids=86525 KVM

@JoachimHenze
Copy link
Contributor

There is one thing that I do not understand yet, and that I want to understand before accepting the PR:
Before your changes there were 3 tests.
WHS passed 3/3 https://reactos.org/testman/compare.php?ids=86181
VBox only 2/3 https://reactos.org/testman/compare.php?ids=86524
KVM only 2/3 https://reactos.org/testman/compare.php?ids=86525

After your changes all: Ros and WHS does succeed all tests. So it appears to me at first glance that the tests are more relaxed now, as in "hiding differences between ros and WHS that are actually there".
But I didn't spot in your code changes yet, where you (weakened or fixed) the already existing test!
Can you elaborate or explain that?

@Doug-Lyons
Copy link
Contributor Author

Doug-Lyons commented Mar 22, 2023 •
edited

Testbot results of current PR:
ExtractIconEx_improve_07u.patch JID65462 on top of 0.4.15-dev-5858-g16decc6

VBox: https://reactos.org/testman/compare.php?ids=86528,86532
KVM: https://reactos.org/testman/compare.php?ids=86527,86531

The single failure is still there and in the new results, it is the first line as follows:
ExtractIconEx.cpp:106: Test failed: ExtractIconExW(0): Expects 1 icons, got 0

@katahiromz katahiromz added the enhancement For PRs with an enhancement/new feature. label Mar 28, 2023
@katahiromz katahiromz added this to New PRs in ReactOS PRs via automation Mar 28, 2023
@Doug-Lyons Doug-Lyons mentioned this pull request Mar 30, 2023
Merged
@JoachimHenze JoachimHenze merged commit c8fc826 into reactos:master Mar 30, 2023
ReactOS PRs automation moved this from New PRs to Done Mar 30, 2023
@Doug-Lyons Doug-Lyons deleted the shell32_apitest_ExtractIconEx_additions branch May 20, 2023 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katahiromz katahiromz katahiromz approved these changes

@JoachimHenze JoachimHenze JoachimHenze approved these changes

@X64X2 X64X2 X64X2 approved these changes

@HBelusca HBelusca Awaiting requested review from HBelusca

Assignees
No one assigned
Labels
enhancement For PRs with an enhancement/new feature. ROSTESTS Label for ROS testcases PRs.
Projects
ReactOS PRs
  
Done
Milestone
No milestone
5 participants
@Doug-Lyons @JoachimHenze @HBelusca @katahiromz @X64X2