Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(809)

Issue 946183002: Add base_unittests to win8 trybot. Fix PEImage tests. (Closed)

Created:
5 years, 10 months ago by Will Harris
Modified:
5 years, 9 months ago
CC:
chromium-reviews, Nico
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add base_unittests to win8 trybot. Fix PEImage tests. Two fixes were required to work on win8: 1. To determine if a directory is a reparse point, use GetFileInformation() since using WIN32_FIND_DATA.dwFileAttributes doesn't work on Windows 8. See https://msdn.microsoft.com/en-us/library/windows/desktop/aa363940.aspx 2. Update the PE image tests to use its own binary, change the expectation format, add support for Win 8.1 and re-enable on 64-bit. BUG=373973, 167707 Committed: https://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27 Cr-Commit-Position: refs/heads/master@{#317960}

Patch Set 1 #

Patch Set 2 : fix base_unittests #

Patch Set 3 : testing bots #

Patch Set 4 : bots are win8.1 #

Patch Set 5 : enable 64-bit. win8 values. #

Patch Set 6 : merge 948593004 #

Patch Set 7 : expectation logic fix #

Total comments: 45

Patch Set 8 : code review comments. #

Patch Set 9 : fix expectations #

Total comments: 6

Patch Set 10 : code review comments #

Patch Set 11 : namespace the callbacks #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+269 lines, -152 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 2 chunks +21 lines, -0 lines 0 comments Download
M base/base.gyp View 1 2 3 4 5 6 7 2 chunks +23 lines, -0 lines 0 comments Download
M base/base_unittests.isolate View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M base/files/file_enumerator_win.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
A base/win/pe_image_test.cc View 1 2 3 4 5 6 7 8 9 1 chunk +31 lines, -0 lines 0 comments Download
M base/win/pe_image_unittest.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +184 lines, -151 lines 1 comment Download
M testing/buildbot/chromium_win8_trybot.json View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 27 (7 generated)
Will Harris
PTAL cpu - fixes to base_unittests dpranke - win8_chromium_rel trybot config
5 years, 10 months ago (2015-02-23 00:09:59 UTC) #2
Will Harris
FYI - I'm working in another CL - to completely rework the PEImage tests - ...
5 years, 10 months ago (2015-02-23 00:42:29 UTC) #3
Will Harris
On 2015/02/23 00:42:29, Will Harris wrote: > FYI - I'm working in another CL - ...
5 years, 10 months ago (2015-02-23 08:35:57 UTC) #4
Dirk Pranke
lgtm
5 years, 10 months ago (2015-02-23 18:54:26 UTC) #5
cpu_(ooo_6.6-7.5)
Rick is the official reviewer of the PE code.
5 years, 10 months ago (2015-02-23 21:07:04 UTC) #7
rvargas (doing something else)
https://codereview.chromium.org/946183002/diff/120001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/946183002/diff/120001/base/base.gyp#newcode1543 base/base.gyp:1543: 'target_name': 'pe_image_test', build.gn? https://codereview.chromium.org/946183002/diff/120001/base/files/file_enumerator_win.cc File base/files/file_enumerator_win.cc (right): https://codereview.chromium.org/946183002/diff/120001/base/files/file_enumerator_win.cc#newcode150 base/files/file_enumerator_win.cc:150: ...
5 years, 10 months ago (2015-02-24 01:52:21 UTC) #8
Will Harris
Thanks for the review, I'll address these comments tomorrow. Before then - a clarification - ...
5 years, 10 months ago (2015-02-24 02:47:07 UTC) #9
rvargas (doing something else)
On 2015/02/24 02:47:07, Will Harris wrote: > Thanks for the review, I'll address these comments ...
5 years, 10 months ago (2015-02-24 03:36:56 UTC) #10
Will Harris
PTAL - The formatting came from clang format. I agree with you that the lack ...
5 years, 10 months ago (2015-02-24 19:00:29 UTC) #11
rvargas (doing something else)
https://codereview.chromium.org/946183002/diff/120001/base/win/pe_image_unittest.cc File base/win/pe_image_unittest.cc (right): https://codereview.chromium.org/946183002/diff/120001/base/win/pe_image_unittest.cc#newcode18 base/win/pe_image_unittest.cc:18: class Expectations { On 2015/02/24 19:00:28, Will Harris wrote: ...
5 years, 10 months ago (2015-02-24 20:03:22 UTC) #12
Will Harris
Think I got everything. PTAL. https://codereview.chromium.org/946183002/diff/160001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/946183002/diff/160001/base/BUILD.gn#newcode1394 base/BUILD.gn:1394: ":pe_image_test", On 2015/02/24 20:03:22, ...
5 years, 10 months ago (2015-02-24 20:46:55 UTC) #13
rvargas (doing something else)
LGTM
5 years, 10 months ago (2015-02-24 22:39:44 UTC) #14
Will Harris
I'll keep a close eye on the waterfall, it's possible one of the bots there ...
5 years, 10 months ago (2015-02-24 23:16:31 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/946183002/200001
5 years, 10 months ago (2015-02-24 23:17:31 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng/builds/14272) ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED, ...
5 years, 10 months ago (2015-02-24 23:35:53 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/946183002/200001
5 years, 10 months ago (2015-02-25 00:49:30 UTC) #22
commit-bot: I haz the power
Committed patchset #11 (id:200001)
5 years, 10 months ago (2015-02-25 03:02:50 UTC) #23
commit-bot: I haz the power
Patchset 11 (id:??) landed as https://crrev.com/b820f17a3767400dac00242c7ee5a948088a0b27 Cr-Commit-Position: refs/heads/master@{#317960}
5 years, 10 months ago (2015-02-25 03:03:32 UTC) #24
Nico
https://codereview.chromium.org/946183002/diff/200001/base/win/pe_image_unittest.cc File base/win/pe_image_unittest.cc (right): https://codereview.chromium.org/946183002/diff/200001/base/win/pe_image_unittest.cc#newcode279 base/win/pe_image_unittest.cc:279: EXPECT_EQ(expectations.GetExpectation(Expectations::RELOCS), count); This fails with clang: PEImageTest.EnumeratesPE (run #1): ...
5 years, 9 months ago (2015-02-25 16:15:15 UTC) #26
Will Harris
5 years, 9 months ago (2015-02-25 17:15:58 UTC) #27
Message was sent while issue was closed.
On 2015/02/25 16:15:15, Nico wrote:
>
https://codereview.chromium.org/946183002/diff/200001/base/win/pe_image_unitt...
> File base/win/pe_image_unittest.cc (right):
> 
>
https://codereview.chromium.org/946183002/diff/200001/base/win/pe_image_unitt...
> base/win/pe_image_unittest.cc:279:
> EXPECT_EQ(expectations.GetExpectation(Expectations::RELOCS), count);
> This fails with clang:
> 
> PEImageTest.EnumeratesPE (run #1):
> [ RUN      ] PEImageTest.EnumeratesPE
> ..\..\base\win\pe_image_unittest.cc(279): error: Value of: count
>   Actual: 1590
> Expected: expectations.GetExpectation(Expectations::RELOCS)
> Which is: 1586
> [  FAILED  ] PEImageTest.EnumeratesPE (0 ms)
> 
>
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/...
> 
> 
> What does this failure mean? Is the code under test wrong? The test? The
> compiler?

I'd be interested in knowing what the additional four relocs are, but the test
expectation format is flexible enough to allow more than one valid "correct"
value if we just want to go ahead and add 1590 OR 1586 as both valid values.

I'll try and compile pe_image_test.dll with clang and see if I can work out
exactly what the difference is.  Can we move the conversation to
crbug.com/167707 for better visibility?  Thanks!

Powered by Google App Engine
This is Rietveld 408576698