Bug 59078

Maybe, adding a new option for reftests is better than re-using 'pixel-tests' option.

Created attachment 90690 [details] skip-reftests-if-pixel-tests-are-disabled

Until we have a new option which is specific to reftests, we should respect 'pixel-tests' option whether we execute reftests or not.

Comment on attachment 90690 [details] skip-reftests-if-pixel-tests-are-disabled View in context: https://bugs.webkit.org/attachment.cgi?id=90690&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:254 > + reftests_files = set(test_file for test_file in self._test_files if self._port.is_reftest(test_file)) > + self._printer.print_expected('%d reftests will be skipped because pixel tests are disabled.' % len(reftests_files)) > + self._test_files -= reftests_files This is a lot of file-system reading just to determine a small number of tests to read. The problem is that we're blocking running any tests on this. I'd rather we not spit out the number of tests skipped and instead skip the test when we're about to run it.

Created attachment 92780 [details] update1

That's a nice point. Actually, the code took about 6 seconds on my machine if we try to run tests for 'LayoutTest/' directory. This is unacceptable, I guess. I updated the patch so that it skips reftests when we're about to run it. (In reply to comment #4) > (From update of attachment 90690 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90690&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:254 > > + reftests_files = set(test_file for test_file in self._test_files if self._port.is_reftest(test_file)) > > + self._printer.print_expected('%d reftests will be skipped because pixel tests are disabled.' % len(reftests_files)) > > + self._test_files -= reftests_files > > This is a lot of file-system reading just to determine a small number of tests to read. The problem is that we're blocking running any tests on this. I'd rather we not spit out the number of tests skipped and instead skip the test when we're about to run it.

Comment on attachment 92780 [details] update1 View in context: https://bugs.webkit.org/attachment.cgi?id=92780&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:122 > + # Skips reftests if pixel tests are disabled. This comment isn't very useful. It just describes what the code does. I think it's fine to just delete it.

Thank you for the review. I'll land the patch after deleting the comment. (In reply to comment #7) > (From update of attachment 92780 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92780&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:122 > > + # Skips reftests if pixel tests are disabled. > > This comment isn't very useful. It just describes what the code does. I think it's fine to just delete it.

Committed r86126: <http://trac.webkit.org/changeset/86126>

http://trac.webkit.org/changeset/86126 might have broken GTK Linux 32-bit Release

Why would disabling pixel tests disable ref-tests? Are reftests slow?

I agree that it is confusing that the 'pixel tests' option affects 'reftests'. That is not an ideal situation. Let me explain the reason. Reftests are similar to 'pixel tests' in concept. In fact, reftests are implementd in a very similar way to 'pixel tests'. In the current implementation, reftests relies on 'pixel test' mechanism. So disabling pixel tests break reftests. I don't think this is an ideal situation. I appreciate if you could consider this is a temporary solution. We should have a better solution, such as a dedicated a command line option for reftests. (In reply to comment #11) > Why would disabling pixel tests disable ref-tests? Are reftests slow?

(In reply to comment #12) > I agree that it is confusing that the 'pixel tests' option affects 'reftests'. > That is not an ideal situation. Let me explain the reason. > > Reftests are similar to 'pixel tests' in concept. In fact, reftests are implementd in a very similar way to 'pixel tests'. > In the current implementation, reftests relies on 'pixel test' mechanism. So disabling pixel tests break reftests. > > I don't think this is an ideal situation. I appreciate if you could consider this is a temporary solution. > We should have a better solution, such as a dedicated a command line option for retests. This reasoning seems a little off to me. Even though pixel tests and reftests are similar from an implementation standpoint, they are not at all similar from a user standpoint. And the user is what matters for command-line options. Pixel tests are typically disabled for some combination of the following reasons: 1) They tend to give a lot of false failures 2) They slow down test runs Presumably (1) does not apply to reftests. (Indeed, that's the whole point!) Does (2) apply, as Eric asked above? If (2) does apply, then it probably makes sense to have a --no-reftests option. Regardless of whether (2) applies, I don't think it makes sense to lump reftests in with pixel tests. They are very different!

I agree with Adam and Eric. The whole point of reftests is that we should be able to use them when we don't want pixel tests but do care about the graphical output, so we shouldn't be disabling them when we disable pixel tests. As to them being slow, logically they'll be a little less than twice as slow as a regular test, but given that we have all of about three of them in the tree, I don't think this is a significant concern at this point. Plus, I'm not a big fan of disabling or skipping tests because they're slow, period.

I agree with Dirk. Though I listed slowness as a reason for pixel tests being disabled, I doubt that is the primary reason. (I think false failures is the primary reason.)

I totally agree. This patch is a just a quick fix for 'reftest breakage'. I just thought that skipping reftests is better than giving wrong test results until we have a dedicated command line option and fix internal implementations. I'll file a another bug to modify the current implementation so that reftests seems to be independent from 'pixel tests'. (In reply to comment #15) > I agree with Dirk. Though I listed slowness as a reason for pixel tests being disabled, I doubt that is the primary reason. (I think false failures is the primary reason.) (In reply to comment #13) > (In reply to comment #12) > > I agree that it is confusing that the 'pixel tests' option affects 'reftests'. > > That is not an ideal situation. Let me explain the reason. > > > > Reftests are similar to 'pixel tests' in concept. In fact, reftests are implementd in a very similar way to 'pixel tests'. > > In the current implementation, reftests relies on 'pixel test' mechanism. So disabling pixel tests break reftests. > > > > I don't think this is an ideal situation. I appreciate if you could consider this is a temporary solution. > > We should have a better solution, such as a dedicated a command line option for retests. > > This reasoning seems a little off to me. Even though pixel tests and reftests are similar from an implementation standpoint, they are not at all similar from a user standpoint. And the user is what matters for command-line options. > > Pixel tests are typically disabled for some combination of the following reasons: > 1) They tend to give a lot of false failures > 2) They slow down test runs > > Presumably (1) does not apply to reftests. (Indeed, that's the whole point!) Does (2) apply, as Eric asked above? > > If (2) does apply, then it probably makes sense to have a --no-reftests option. Regardless of whether (2) applies, I don't think it makes sense to lump reftests in with pixel tests. They are very different!

Filed as https://bugs.webkit.org/show_bug.cgi?id=60605. (In reply to comment #16) > I totally agree. This patch is a just a quick fix for 'reftest breakage'. > I just thought that skipping reftests is better than giving wrong test results until we have a dedicated command line option and fix internal implementations. > > I'll file a another bug to modify the current implementation so that reftests seems to be independent from 'pixel tests'. > > (In reply to comment #15) > > I agree with Dirk. Though I listed slowness as a reason for pixel tests being disabled, I doubt that is the primary reason. (I think false failures is the primary reason.) > > (In reply to comment #13) > > (In reply to comment #12) > > > I agree that it is confusing that the 'pixel tests' option affects 'reftests'. > > > That is not an ideal situation. Let me explain the reason. > > > > > > Reftests are similar to 'pixel tests' in concept. In fact, reftests are implementd in a very similar way to 'pixel tests'. > > > In the current implementation, reftests relies on 'pixel test' mechanism. So disabling pixel tests break reftests. > > > > > > I don't think this is an ideal situation. I appreciate if you could consider this is a temporary solution. > > > We should have a better solution, such as a dedicated a command line option for retests. > > > > This reasoning seems a little off to me. Even though pixel tests and reftests are similar from an implementation standpoint, they are not at all similar from a user standpoint. And the user is what matters for command-line options. > > > > Pixel tests are typically disabled for some combination of the following reasons: > > 1) They tend to give a lot of false failures > > 2) They slow down test runs > > > > Presumably (1) does not apply to reftests. (Indeed, that's the whole point!) Does (2) apply, as Eric asked above? > > > > If (2) does apply, then it probably makes sense to have a --no-reftests option. Regardless of whether (2) applies, I don't think it makes sense to lump reftests in with pixel tests. They are very different!