Bug 83176 – gtk_unittest.GtkPortTest.test_get_crash_log failing on windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Tony Chang]

http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/25106/steps/webkitpy-test/logs/stdio

Looks like it's been failing since it was added in http://trac.webkit.org/changeset/113037 .

Should we skip this test on Windows?

[Philippe Normand]

Oh sorry about that!

Yeah I guess it's safe to not run this test on non-linux platforms.

[Philippe Normand]

Created attachment 135618 [details]
Patch

[Tony Chang]

Comment on attachment 135618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135618&action=review

> Tools/Scripts/webkitpy/layout_tests/port/gtk_unittest.py:72
> +        if sys.platform != "linux2":

What about linux3 (I think newer kernels show up as linux3)?  I normally just do sys.platform.startswith("linux").

[Philippe Normand]

Committed r113210: <http://trac.webkit.org/changeset/113210>

[Philippe Normand]

(In reply to comment #3)
> (From update of attachment 135618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135618&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/gtk_unittest.py:72
> > +        if sys.platform != "linux2":
> 
> What about linux3 (I think newer kernels show up as linux3)?  I normally just do sys.platform.startswith("linux").

Oh I didn't know about that linux3 :) I landed the modified patch. Thanks!

[Dirk Pranke]

I'm generally not a fan of skipping unit tests on different platforms; if this test is failing on windows, it suggests that it's either not correctly written or it depends on something in the environment or operating system. If it's the latter, we should probably rename this to integration_test_get_crash_log() to be clearer ...

[Philippe Normand]

(In reply to comment #6)
> I'm generally not a fan of skipping unit tests on different platforms; if this test is failing on windows, it suggests that it's either not correctly written or it depends on something in the environment or operating system. If it's the latter, we should probably rename this to integration_test_get_crash_log() to be clearer ...

Indeed I was too quick skipping this on non-linux, the mock crash log used in the test is using  %(core_directory)s/core-pid_%%p-_-process_%%e instead of building the path with os.path.join().

I'll send a new patch.

[Philippe Normand]

Created attachment 135771 [details]
Patch

[Tony Chang]

Comment on attachment 135771 [details]
Patch

If GtkPort._get_crash_log used FileSystem.join and you used MockFileSystem in the test, you would always get / when running the test since MockFileSystem always uses / as the separator.

This seems fine too.

[Dirk Pranke]

Comment on attachment 135771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135771&action=review

> Tools/Scripts/webkitpy/layout_tests/port/gtk_unittest.py:60
> +        core_pattern = os.path.join(core_directory, "core-pid_%p-_-process_%e")

This should probably be port._filesystem.join() (and moved below the self.make_port() call), else you'll get diffs on windows from backslashes in the path, I expect.

[WebKit Review Bot]

Comment on attachment 135771 [details]
Patch

Clearing flags on attachment: 135771

Committed r113339: <http://trac.webkit.org/changeset/113339>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.