Skip tests that create symbol links when not allowed [crashpad/crashpad : main]

0 views
Skip to first unread message

Bruce Dawson (Gerrit)

unread,
Mar 17, 2023, 6:43:04 PM3/17/23
to Mark Mentovai, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Mark Mentovai.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      PTAL - simple fix to a couple of incorrect test conditions.

To view, visit change 4348801. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: main
Gerrit-Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
Gerrit-Change-Number: 4348801
Gerrit-PatchSet: 2
Gerrit-Owner: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Attention: Mark Mentovai <ma...@chromium.org>
Gerrit-Comment-Date: Fri, 17 Mar 2023 22:43:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Mark Mentovai (Gerrit)

unread,
Mar 17, 2023, 7:03:57 PM3/17/23
to Bruce Dawson, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Bruce Dawson.

View Change

1 comment:

  • File util/file/filesystem_test.cc:

    • Patch Set #2, Line 480: GTEST_SKIP();

      I don't think we need to skip the whole test. We can just skip the parts that involve symbolic links. Follow the `!IS_FUCHSIA` lead below: Fuchsia doesn't have symbolic links either. Applies to both tests.

To view, visit change 4348801. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: main
Gerrit-Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
Gerrit-Change-Number: 4348801
Gerrit-PatchSet: 2
Gerrit-Owner: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Attention: Bruce Dawson <bruce...@chromium.org>
Gerrit-Comment-Date: Fri, 17 Mar 2023 23:03:54 +0000

Bruce Dawson (Gerrit)

unread,
Mar 17, 2023, 7:08:51 PM3/17/23
to Mark Mentovai, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Mark Mentovai.

View Change

1 comment:

  • File util/file/filesystem_test.cc:

    • I don't think we need to skip the whole test. […]

      Good point. Done.

To view, visit change 4348801. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: main
Gerrit-Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
Gerrit-Change-Number: 4348801
Gerrit-PatchSet: 3
Gerrit-Owner: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Attention: Mark Mentovai <ma...@chromium.org>
Gerrit-Comment-Date: Fri, 17 Mar 2023 23:08:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Mark Mentovai <ma...@chromium.org>
Gerrit-MessageType: comment

Mark Mentovai (Gerrit)

unread,
Mar 17, 2023, 8:03:01 PM3/17/23
to Bruce Dawson, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Bruce Dawson.

View Change

1 comment:

  • File util/file/filesystem_test.cc:

    • Patch Set #3, Line 538: EXPECT_EQ(filesize, 2 * sizeof(kTestFileContent));

      We really do want to reach this, it's the whole point of the test, and it doesn't depend on symbolic links. So no `GTEST_SKIP` in this one, just make the stuff that's preprocessored out on Fuchsia be skipped at runtime when symbolic links aren't supported.

To view, visit change 4348801. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: main
Gerrit-Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
Gerrit-Change-Number: 4348801
Gerrit-PatchSet: 3
Gerrit-Owner: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Attention: Bruce Dawson <bruce...@chromium.org>
Gerrit-Comment-Date: Sat, 18 Mar 2023 00:02:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Bruce Dawson (Gerrit)

unread,
Mar 17, 2023, 11:55:50 PM3/17/23
to Mark Mentovai, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Mark Mentovai.

View Change

1 comment:

  • File util/file/filesystem_test.cc:

    • We really do want to reach this, it's the whole point of the test, and it doesn't depend on symbolic […]

      Gotcha. I didn't notice the code at the end after the #ifdef. Fixed.

To view, visit change 4348801. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: main
Gerrit-Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
Gerrit-Change-Number: 4348801
Gerrit-PatchSet: 4
Gerrit-Owner: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Attention: Mark Mentovai <ma...@chromium.org>
Gerrit-Comment-Date: Sat, 18 Mar 2023 03:55:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Mark Mentovai (Gerrit)

unread,
Mar 18, 2023, 1:03:53 AM3/18/23
to Bruce Dawson, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Bruce Dawson.

Patch set 4:Code-Review +1

View Change

1 comment:

To view, visit change 4348801. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: main
Gerrit-Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
Gerrit-Change-Number: 4348801
Gerrit-PatchSet: 4
Gerrit-Owner: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Bruce Dawson <bruce...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Attention: Bruce Dawson <bruce...@chromium.org>
Gerrit-Comment-Date: Sat, 18 Mar 2023 05:03:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Bruce Dawson (Gerrit)

unread,
Mar 18, 2023, 1:36:04 AM3/18/23
to Mark Mentovai, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Bruce Dawson.

Patch set 4:Commit-Queue +2

View Change

    To view, visit change 4348801. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: crashpad/crashpad
    Gerrit-Branch: main
    Gerrit-Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
    Gerrit-Change-Number: 4348801
    Gerrit-PatchSet: 4
    Gerrit-Owner: Bruce Dawson <bruce...@chromium.org>
    Gerrit-Reviewer: Bruce Dawson <bruce...@chromium.org>
    Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
    Gerrit-Attention: Bruce Dawson <bruce...@chromium.org>
    Gerrit-Comment-Date: Sat, 18 Mar 2023 05:36:01 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    Gerrit-MessageType: comment

    Crashpad LUCI CQ (Gerrit)

    unread,
    Mar 18, 2023, 1:36:09 AM3/18/23
    to Bruce Dawson, Mark Mentovai, crashp...@chromium.org

    Crashpad LUCI CQ submitted this change.

    View Change

    Approvals: Bruce Dawson: Commit Mark Mentovai: Looks good to me
    Skip tests that create symbol links when not allowed

    Several tests in filesystem_test.cc create symbol links. The privilege
    needed to do this is not enabled on all Windows systems so several of
    the tests check for the privilege and are skipped if it is not
    available.

    However, two tests that created symbol links were not doing this check
    and therefore failed on some Windows machines. This corrects those
    failures by adding the checks.

    Bug: chromium:1418165
    Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
    Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4348801
    Reviewed-by: Mark Mentovai <ma...@chromium.org>
    Commit-Queue: Bruce Dawson <bruce...@chromium.org>
    ---
    M util/file/filesystem_test.cc
    1 file changed, 12 insertions(+), 6 deletions(-)

    diff --git a/util/file/filesystem_test.cc b/util/file/filesystem_test.cc
    index eb492cd..bff3e32 100644
    --- a/util/file/filesystem_test.cc
    +++ b/util/file/filesystem_test.cc
    @@ -487,6 +487,10 @@
    EXPECT_EQ(filesize, sizeof(kTestFileContent));

    #if !BUILDFLAG(IS_FUCHSIA)
    + if (!CanCreateSymbolicLinks()) {
    + GTEST_SKIP();
    + }
    +
    // Create a link to a file.
    base::FilePath link(temp_dir.path().Append(FILE_PATH_LITERAL("link")));
    ASSERT_TRUE(CreateSymbolicLink(filepath, link));
    @@ -517,13 +521,15 @@
    writer2.Close();

    #if !BUILDFLAG(IS_FUCHSIA)
    - // Create a link to a file.
    - base::FilePath link(dir.Append(FILE_PATH_LITERAL("link")));
    - ASSERT_TRUE(CreateSymbolicLink(filepath2, link));
    + if (CanCreateSymbolicLinks()) {
    + // Create a link to a file.
    + base::FilePath link(dir.Append(FILE_PATH_LITERAL("link")));
    + ASSERT_TRUE(CreateSymbolicLink(filepath2, link));

    - // Create a link to a dir.
    - base::FilePath linkdir(temp_dir.path().Append(FILE_PATH_LITERAL("link")));
    - ASSERT_TRUE(CreateSymbolicLink(dir, linkdir));
    + // Create a link to a dir.
    + base::FilePath linkdir(temp_dir.path().Append(FILE_PATH_LITERAL("link")));
    + ASSERT_TRUE(CreateSymbolicLink(dir, linkdir));
    + }
    #endif // !BUILDFLAG(IS_FUCHSIA)

    uint64_t filesize = GetDirectorySize(temp_dir.path());

    To view, visit change 4348801. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: crashpad/crashpad
    Gerrit-Branch: main
    Gerrit-Change-Id: I6621796b462b8db02271ad5a05e0c29ee047f648
    Gerrit-Change-Number: 4348801
    Gerrit-PatchSet: 5
    Gerrit-Owner: Bruce Dawson <bruce...@chromium.org>
    Gerrit-Reviewer: Bruce Dawson <bruce...@chromium.org>
    Gerrit-Reviewer: Crashpad LUCI CQ <crashpa...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
    Gerrit-MessageType: merged
    Reply all
    Reply to author
    Forward
    0 new messages