Bug 53756 – Refactor and/or split and/or rename the extend-selection tests to be clearer what they're doing, and able to be run manually

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin (Ben) Kalman]

Refactor and/or split and/or rename the extend-selection tests to be clearer what they're doing, and able to be run manually

[Benjamin (Ben) Kalman]

Created attachment 81186 [details]
Patch

[Benjamin (Ben) Kalman]

Umm, patch didn't seem to upload correctly.  I will try again.

[Ryosuke Niwa]

Comment on attachment 81186 [details]
Patch

Making tests manually runnable is good but all these rewrites of script isn't that great.  It's really hard to see that new tests are testing the same things the old tests were testing.  In general, we must be careful when modifying tests because subtle differences may affect what tests do.  At least, we should separate refactoring and changes to make it manually runnable.

I don't think separating the line boundary tests into LTR/RTL tests is a good idea because having both results on one test has helped us catching regressions in either result since extend-selection-* tests are written so that LTR/RTL results should match.

[Benjamin (Ben) Kalman]

> Making tests manually runnable is good but all these rewrites of script isn't that great.  It's really hard to see that new tests are testing the same things the old tests were testing.  In general, we must be careful when modifying tests because subtle differences may affect what tests do.  At least, we should separate refactoring and changes to make it manually runnable.

If you look at the changes to the expected results it's pretty clear that it's testing the same thing.

> 
> I don't think separating the line boundary tests into LTR/RTL tests is a good idea because having both results on one test has helped us catching regressions in either result since extend-selection-* tests are written so that LTR/RTL results should match.

The reason I split those into LTR and RTL tests is because it's impossible to run them manually without doing that.  The transition from LTR to RTL actually modifies the dom mid-test.

[Benjamin (Ben) Kalman]

Created attachment 81190 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #4)
> > Making tests manually runnable is good but all these rewrites of script isn't that great.  It's really hard to see that new tests are testing the same things the old tests were testing.  In general, we must be careful when modifying tests because subtle differences may affect what tests do.  At least, we should separate refactoring and changes to make it manually runnable.
> 
> If you look at the changes to the expected results it's pretty clear that it's testing the same thing.

Sure expected tests are same but extend-selection-* tests relies heavily on scripts so I wouldn't modify like this all at once.

> > I don't think separating the line boundary tests into LTR/RTL tests is a good idea because having both results on one test has helped us catching regressions in either result since extend-selection-* tests are written so that LTR/RTL results should match.
> 
> The reason I split those into LTR and RTL tests is because it's impossible to run them manually without doing that.  The transition from LTR to RTL actually modifies the dom mid-test.

We need to find a better way of testing both LTR and RTL cases then.  I don't think it's acceptable to separate LTR/RTL tests; we'll have regressions in no time if we did that.

[Benjamin (Ben) Kalman]

(In reply to comment #6)
> (In reply to comment #4)
> > > Making tests manually runnable is good but all these rewrites of script isn't that great.  It's really hard to see that new tests are testing the same things the old tests were testing.  In general, we must be careful when modifying tests because subtle differences may affect what tests do.  At least, we should separate refactoring and changes to make it manually runnable.
> > 
> > If you look at the changes to the expected results it's pretty clear that it's testing the same thing.
> 
> Sure expected tests are same but extend-selection-* tests relies heavily on scripts so I wouldn't modify like this all at once.

I don't think I know what you mean?  What would the value in splitting up a change like this?  Changes to the script are necessary, and whether they're made all at once or incrementally the end result is the same, that is, there would be the same amount of doubt that they're still reporting correctly.

And I think the chances of them being different are quite low given
 - they're the same now
 - for any incorrect cases, the code is simpler now.

> 
> > > I don't think separating the line boundary tests into LTR/RTL tests is a good idea because having both results on one test has helped us catching regressions in either result since extend-selection-* tests are written so that LTR/RTL results should match.
> > 
> > The reason I split those into LTR and RTL tests is because it's impossible to run them manually without doing that.  The transition from LTR to RTL actually modifies the dom mid-test.
> 
> We need to find a better way of testing both LTR and RTL cases then.  I don't think it's acceptable to separate LTR/RTL tests; we'll have regressions in no time if we did that.

We could have both the LTR and RTL doms in the same test, but when I did that there were ~50 divs on the page and it becomes more difficult to spot what actually went wrong.  It's a tradeoff I guess.

I still don't know how having them in separate tests implies regressions, though.

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #6)
> I don't think I know what you mean?  What would the value in splitting up a change like this?  Changes to the script are necessary, and whether they're made all at once or incrementally the end result is the same, that is, there would be the same amount of doubt that they're still reporting correctly.
> 
> And I think the chances of them being different are quite low given
>  - they're the same now
>  - for any incorrect cases, the code is simpler now.

Let's make more incremental changes.  We always prefer to have many small changes over having one big patch as long as each change is coherent.

> > We need to find a better way of testing both LTR and RTL cases then.  I don't think it's acceptable to separate LTR/RTL tests; we'll have regressions in no time if we did that.
> 
> We could have both the LTR and RTL doms in the same test, but when I did that there were ~50 divs on the page and it becomes more difficult to spot what actually went wrong.  It's a tradeoff I guess.

We could separate tests in line boundary into more categories if you wish.

> I still don't know how having them in separate tests implies regressions, though.

Because extend-selection* doesn't print PASS/FAIL, there have been multiple occasions where people just rebaselined the expected result when the test started to fail on non-Mac platforms instead of actually fixing them.  And if we had LTR/RTL results on separate files, people are very unlikely to realize that those results supposed to match.

[Benjamin (Ben) Kalman]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > I don't think I know what you mean?  What would the value in splitting up a change like this?  Changes to the script are necessary, and whether they're made all at once or incrementally the end result is the same, that is, there would be the same amount of doubt that they're still reporting correctly.
> > 
> > And I think the chances of them being different are quite low given
> >  - they're the same now
> >  - for any incorrect cases, the code is simpler now.
> 
> Let's make more incremental changes.  We always prefer to have many small changes over having one big patch as long as each change is coherent.

Yes exactly, so long as each change is coherent.  If you look at the patch it's got a lot of renaming to make the vocabulary more consistent, code simplification, etc that spans the html as well as js files.  It would an unreasonable amount of effort to try to separate them.

> 
> > > We need to find a better way of testing both LTR and RTL cases then.  I don't think it's acceptable to separate LTR/RTL tests; we'll have regressions in no time if we did that.
> > 
> > We could have both the LTR and RTL doms in the same test, but when I did that there were ~50 divs on the page and it becomes more difficult to spot what actually went wrong.  It's a tradeoff I guess.
> 
> We could separate tests in line boundary into more categories if you wish.

After this change I hope to make the tests more clearly targeted, if possible.  I just want to make them manually testable for now, to help with another patch I'm working on :)

> 
> > I still don't know how having them in separate tests implies regressions, though.
> 
> Because extend-selection* doesn't print PASS/FAIL, there have been multiple occasions where people just rebaselined the expected result when the test started to fail on non-Mac platforms instead of actually fixing them.  And if we had LTR/RTL results on separate files, people are very unlikely to realize that those results supposed to match.

Ok I agree, thanks.

[Ryosuke Niwa]

(In reply to comment #9)
> (In reply to comment #8)
> > Let's make more incremental changes.  We always prefer to have many small changes over having one big patch as long as each change is coherent.
> 
> Yes exactly, so long as each change is coherent.  If you look at the patch it's got a lot of renaming to make the vocabulary more consistent, code simplification, etc that spans the html as well as js files.  It would an unreasonable amount of effort to try to separate them.

We should do cleanup first.  You can even separate renaming into a separate patch.  That way, each patch's correctness will be self-evident.  Once we cleaned up the test, then we can split the tests or whatever you please to do.

In general, working on WebKit requires making changes locally and then splitting the changes into cleanup/refactoring and actual fix because it's patch author's responsibility to make sure the patch is small (~20KB) and changes are easy to review.

> > We could separate tests in line boundary into more categories if you wish.
> 
> After this change I hope to make the tests more clearly targeted, if possible.  I just want to make them manually testable for now, to help with another patch I'm working on :)

Yeah, separating tests into smaller tests will be nice because we'll be able to diagnose the issues more easily when they fail.

[Ryosuke Niwa]

Comment on attachment 81190 [details]
Patch

r- per discussion on the bug.

[Benjamin (Ben) Kalman]

This is now redundant, fixed via other patches.