Closed Bug 344591 (reftest) Opened 14 years ago Closed 14 years ago

framework for layout acceptance tests with HTML(etc.) reference renderings (reftest)

Categories

(Testing :: Reftest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(7 files)

I'd like us to have a framework for layout acceptance tests that do not involve before-patch and after-patch comparison and don't require fragile reference renderings.

What I propose we have is a framework for running image-comparison tests on HTML (or XML, etc.) pages with the reference renderings also in HTML/XML.  This makes it a bit more work to write the tests, but makes them much less fragile in the end.  They don't need reference renderings (per platform) to be updated for changes in default font preferences, changes in the default HTML style sheet, changes in form control appearance, etc., and they also don't need a baseline run to be useful.

We don't have existing tests that fit into this framework, but we could gradually build up a library of such tests, and I think they'd be much easier to use and to automate than tests that require a baseline run or tests that have reference renderings in bitmap format.  A large number of layout, CSS, and DOM tests could be written such that they fit into this framework, though, so we can accumulate tests across a number of areas.

Martijn -- I've mentioned this to you in the past; I'm not sure if you're working on it.  If you are, please take the bug.  If not, I may get a chance to work on it sometime in the near future.

I think the best implementation strategy will probably involve a very simple chrome app and a small C++ component to do the image comparison (probably much faster than doing string comparison on canvas.toDataURL()).
Yeah, I'll see what I can do. I'm afraid, I still haven't done anything useful in this regard. I'll try to work on it.
Assignee: dbaron → martijn.martijn
Discussed with Martijn on IRC; I'll actually take this back.
Assignee: martijn.martijn → dbaron
Attached file sample manifest file
To run, use "<path/to/firefox> -layouttest <path/to/manifest>" (since I didn't get to that part of the documentation yet).  Manifests have to be local files; tests need not be.
er, -layoutatest, not -layouttest.

But I'm considering changing the name since I don't like it.
I think I'm leaning towards calling this "reftest" since it's tests with reference renderings.

So now I'm trying to think about what directory structure we want the stuff in.  Given our existing structure (markup tests for our old framework are in layout/html/tests/ along with unit tests for other things), I'm not quite sure where things should go.  That directory is already quite polluted and doesn't fit with the existing directory structure, nor does the name make sense (not html-specific at all).

I'm thinking maybe the harness should go in layout/tools/xul-reftest/ and the tests should go in layout/tests/ or layout/reftests/.

But I'm really not happy with any of these ideas.
Since the rendering is done by the rendering test application, any test which causes a crash will break not only the current test but all tests following it.  dbaron says that the intent of this tool is not to test for crashes, but since no crash-testing framework exists yet (that I know of) for layout, I'm concerned that this framework doesn't offer some way of dealing with crashing tests (other than by just commenting them out).
The approach I use is to kick off the test application from a perl script that  invokes the test application via a python script for each of a set of inputs specified on the command line. The python script will timeout the test application if it does not terminate in a specified period of time. It also outputs a recognizable EXIT status to stdout. In the event of a crash of the test application, the driving perl script just moves on to the next set of inputs.
let's get this in to the tree without harnesss code to handle crashing tests, and track crashing test cases somewhere else.

I'm traveling tomorrow, but I'll poke around over the weekend and try to think of suggestions as to where these tests and test harness might live.
I'd vote for the harness in layout/tools/reftest and the tests in layout/tests.
Can't we move the tests out of the normal pull? We should have done that for the old tests already. I see a big discrepancy between the number of people who run the test ( < 10 I guess) and the cvs time burden that we put on everyone.
I think everyone should get in the habit of running all the tests whenever they change anything.  To this end, we should include the tests in the "normal pull".  We should also make the tests run quickly, but that is another issue.

I also believe there are certain instances of automated tests that may not be appropriate to run all the time by everybody.  It might make sense to remove this subset of tests, and test harness stuff specific to these tests, from the "normal pull".

Bernd, do you know of other open-source projects that 

a) contain automated regression tests at the module or unit level, and
b) do not include those tests in the "normal pull"

Most of the ones I've seen *do* include the tests.
I was referring to bug 83934.
I don't know  other open source projects well as this is my favourite OSS project.
I checked in the current versions of the XUL harness into layout/tools/reftest/.
(In reply to comment #11)
> I'd vote for the harness in layout/tools/reftest and the tests in layout/tests.

So I was thinking that maybe the test pages should be in layout/testcases ?  Then again, I'm not sure if a tests vs. testcases distinction really makes sense...
I'm not sure we really need to separate them out. They're probably(In reply to comment #16)
> So I was thinking that maybe the test pages should be in layout/testcases ? 
> Then again, I'm not sure if a tests vs. testcases distinction really makes
> sense...

yeah, I'm not sure that buys us much either. You might consider layout/tests/content for the html? Is there a document somewhere on how to run these?

Is the method to invoke this still -layoutatest or has it changed?

./firefox -reftest <manifest>
ah, thank you both. I had just found the README file.
RobC,

Are you working on integrating this into some invocation harness?  If so, please post or attach what you come up with.  Maybe http://people.mozilla.com/~davel/scripts/ can help as an example of how to set up profiles and install manifests from scratch (look at extract_search.sh)
(In reply to comment #21)

> Are you working on integrating this into some invocation harness?  If so,
> please post or attach what you come up with.  Maybe
> http://people.mozilla.com/~davel/scripts/ can help as an example of how to set
> up profiles and install manifests from scratch (look at extract_search.sh)

I'm certainly thinking about it and think it'd be good to have. Thanks for the link!
Summary: framework for layout acceptance tests with HTML(etc.) reference renderings → framework for layout acceptance tests with HTML(etc.) reference renderings (reftest)
Alias: reftest
David,  this patch builds in reftest when ENABLE_TESTS is true (--disable-tests not specified).  This is slightly easier than running extra make invocations by hand, especially since one has to do even more extra steps to get the files installed in mac builds.

We could also package reftest as an extension, and have folks (and test farms) install it as part of the test setup.

Which approach would you prefer?  Please minus this patch if you want me to go for the extension approach as well as if I got something wrong.

Thanks
Attachment #241140 - Flags: review?(dbaron)
Comment on attachment 241140 [details] [diff] [review]
makefile changes to build and install reftest

This is fine, although we should probably double-check that releases are done with --disable-tests.

That said, the actual code that this is turning on should probably get reviewed at some point, although perhaps it doesn't need to be since it's not part of what we ship.
Attachment #241140 - Flags: review?(dbaron) → review+
I'm going to check a handful of tests into layout/reftests/ in a day or so unless people think one of the other directory names was better.
checked in makefile patch to trunk

Checking in Makefile.in;
/cvsroot/mozilla/layout/Makefile.in,v  <--  Makefile.in
new revision: 1.26; previous revision: 1.25
done
dbaron, could you please remove (or rename) the check target in reftest/Makefile?  It's going to cause my staging trees to go orange.

thanks

_Dave
Attachment #243232 - Flags: review?(dbaron)
Why?  Nothing references that makefile yet, and shouldn't until it works.
because |make check| is not the right target for tests that require an instance of an installed browser.  having that type of test mixed in with the other tests complicates efforts to run these tests on continuous build machines.

I'm afraid someone will see your checkin, and put such a test into another makefile under the check target.
Attachment #243232 - Flags: review?(dbaron) → review+
Attachment #243232 - Flags: review+ → review-
So I definitely want this to be part of something once it works.  What should that be?  There is no recursive |make lcheck| target.
Attachment #243232 - Flags: review- → review+
(In reply to comment #31)
> So I definitely want this to be part of something once it works.  What should
> that be?  There is no recursive |make lcheck| target.

[recording face-to-face discussion here]

We'll have something other than a recursive make target to support running tests that require an installed instance of the app.

We need to make it easy for developers to run these tests, and not overly complicated for these tests to run reliably in an unattended automated manner.
reftest error parsing manifest with comments or blank lines:

TypeError: /\S.*/.exec(str) has no properties

I think this is because the expression does not match if str contains no non-whitespace chars.

regexp used to clear comments from lines sets str to null if '#' is the first character on the line.  When str is null, the regexp to strip leading whitespace fails because str is not a string.

add a test for this condition before the regexp fires, and skip the line if the test is true.
Attachment #243798 - Flags: review?(dbaron)
Comment on attachment 243798 [details] [diff] [review]
fix error parsing comment lines

I'd prefer just doing

if (!str)
  continue;

between the two str = /.../exec(str).[0]; lines, with the comment "// entire line was a comment"
Attachment #243798 - Flags: review?(dbaron) → review+
I'll check in the patch with your suggested change once I test it on my box (typo detection).

BTW, reftest is running continuously (every 6 hours) on my private build machines.  Once IT sets up some VMs, it will be running on public/qa build machines.  see the MozillaTest tree for details.
fix checked in

Checking in reftest.js;
/cvsroot/mozilla/layout/tools/reftest/reftest.js,v  <--  reftest.js
new revision: 1.2; previous revision: 1.1
done
the regexp that stripped trailing comments did not strip whitespace between the last item and the # character.

replace regexp that trimmed leading whitespace with one that trims both leading and trailing whitespace.
Attachment #243870 - Flags: review?(dbaron)
Attachment #243870 - Flags: review?(dbaron) → review+
Comment on attachment 243870 [details] [diff] [review]
trim leading and trailing whitespace before splitting manifest line

checked in to trunk

Checking in reftest.js;
/cvsroot/mozilla/layout/tools/reftest/reftest.js,v  <--  reftest.js
new revision: 1.3; previous revision: 1.2
done
davel has the tests running in an automated way on a buildbot instance on his desktop machine.  At some point we'll hopefully get the results published somewhere public.  But the framework exists, and seems to be usable at this point, so I think it's time to mark this bug fixed.  There's plenty of room for more bug reports in bugzilla.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated the docs: http://developer.mozilla.org/en/docs/index.php?title=Mozilla_automated_testing&diff=43131&oldid=43128

Let me know if I got anything wrong.

Should I file a bug about make lcheck not working on windows due to /cygdrive path being passed instead of an URL or is a better solution being worked on?
Component: Layout: Misc Code → Reftest
Product: Core → Testing
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.