Bug 134724 – fast/css/vertical-text-overflow-ellipsis* provide the wrong expectations

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Text

[Mario Sanchez Prada]

While working on bug 129313 and bug 134597, I found that these tests where actually not printing the right results, as they are not printing the "ellipsis" character at all, as they should.

For instance, taking a look to the PNG images in LayoutTests/platform/mac/fast/css/vertical-text-overflow-ellipsis-text-align-*png you can see how the ellipsis character is not being printed at all. I found this issue while trying to convert all those tests to reftests, so I'm filing this bug now to keep track of the issue, which might be similar to the one in 129313, but I feel like it's better to treat it as a separate one.

PS: Perhaps we should remove all those text expectations that are wrong anyway and add [ Failure ] entries in the TestExpectations file, to keep track of the issue as well in that way

[Ryosuke Niwa]

(In reply to comment #0)
> PS: Perhaps we should remove all those text expectations that are wrong anyway and add [ Failure ] entries in the TestExpectations file, to keep track of the issue as well in that way

That would be worse. With the wrong expectation checked in, we would know when the output changes, adding a test expectation [Failure] would only mask any future changes to the test output.

[Mario Sanchez Prada]

(In reply to comment #1)
> (In reply to comment #0)
> > PS: Perhaps we should remove all those text expectations that are wrong anyway and add [ Failure ] entries in the TestExpectations file, to keep track of the issue as well in that way
> 
> That would be worse. With the wrong expectation checked in, we would know when the output changes, adding a test expectation [Failure] would only mask any future changes to the test output.

What if we remove those wrong expectations and convert the original tests to reftests (so the -expected.html file expects the right thing, hardcoding the ellipsis character) and then add the [ Failure ] expectation?

That way, when the problem is fixed the output of the .html and the -expected.html should match and that should be noticed while running the layout tests.

[Ryosuke Niwa]

(In reply to comment #2)
> What if we remove those wrong expectations and convert the original tests to reftests (so the -expected.html file expects the right thing, hardcoding the ellipsis character) and then add the [ Failure ] expectation?
> 
> That way, when the problem is fixed the output of the .html and the -expected.html should match and that should be noticed while running the layout tests.

We still wouldn't catch new regressions that way.  In general, having any failing line in the test expectations is not great.

[Mario Sanchez Prada]

(In reply to comment #3)
> (In reply to comment #2)
> > What if we remove those wrong expectations and convert the original tests to reftests (so the -expected.html file expects the right thing, hardcoding the ellipsis character) and then add the [ Failure ] expectation?
> > 
> > That way, when the problem is fixed the output of the .html and the -expected.html should match and that should be noticed while running the layout tests.
> 
> We still wouldn't catch new regressions that way.  In general, having any failing line in the test expectations is not great.

I see. But then the problem is that I would need to rebaseline the expectations for at least vertical-text-overflow-ellipsis-text-align-center.html and vertical-text-overflow-ellipsis-text-align-right.html when proposing the patch for 129313, since that will impact slightly how this vertical text is rendered, which (that rebaseline) is what I was trying to avoid with this bug report.

With that in my mind, and even if it's not perfect, I thought converting these vertical text tests to reftest now and marking the failure in TestExpectations would not be a so bad option, but I'm open to other suggestions if you have any.

Perhaps I should try to fix both the horizontal and vertical text at once as part of the patch for 129313, and then we could forget about this bug as all these vertical text tests would be converted along with that patch. What do you think?

[Ryosuke Niwa]

(In reply to comment #4)
> With that in my mind, and even if it's not perfect, I thought converting these vertical text tests to reftest now and marking the failure in TestExpectations would not be a so bad option, but I'm open to other suggestions if you have any.

We shouldn't do this.  It would mean that we're not testing anything since ref tests with ImageOnlyFailure expectation will NOT catch anything but crash.

> Perhaps I should try to fix both the horizontal and vertical text at once as part of the patch for 129313, and then we could forget about this bug as all these vertical text tests would be converted along with that patch. What do you think?

We can just rebaseline the test instead in your patch.

[Mario Sanchez Prada]

(In reply to comment #5)
> (In reply to comment #4)
> > With that in my mind, and even if it's not perfect, I thought converting these vertical text tests to reftest now and marking the failure in TestExpectations would not be a so bad option, but I'm open to other suggestions if you have any.
> 
> We shouldn't do this.  It would mean that we're not testing anything since ref tests with ImageOnlyFailure expectation will NOT catch anything but crash.
> 
> > Perhaps I should try to fix both the horizontal and vertical text at once as part of the patch for 129313, and then we could forget about this bug as all these vertical text tests would be converted along with that patch. What do you think?
> 
> We can just rebaseline the test instead in your patch.

Ok. Sounds good to me too. I'll close this bug then and incorporate those rebaselines in the patch for bug 129313 then.

Thanks!