David Baron :dbaron: Assignee
	Description • 18 years ago

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()).

	Martijn Wargers (dead)
	Comment 1 • 18 years ago

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.

	David Baron :dbaron: Assignee
	Comment 2 • 18 years ago

Discussed with Martijn on IRC; I'll actually take this back.

	David Baron :dbaron: Assignee
	Comment 5 • 18 years ago

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.

	David Baron :dbaron: Assignee
	Comment 6 • 18 years ago

er, -layoutatest, not -layouttest.

But I'm considering changing the name since I don't like it.

	David Baron :dbaron: Assignee
	Comment 7 • 18 years ago

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.

	Jeff Walden [:Waldo]
	Comment 8 • 18 years ago

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).

	Bob Clary [:bc] (inactive)
	Comment 9 • 18 years ago

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.

	Dave Liebreich [:davel]
	Comment 10 • 18 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 18 years ago

I'd vote for the harness in layout/tools/reftest and the tests in layout/tests.

	Bernd
	Comment 12 • 18 years ago

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.

	Dave Liebreich [:davel]
	Comment 13 • 18 years ago

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.

	Bernd
	Comment 14 • 18 years ago

I was referring to bug 83934.
I don't know  other open source projects well as this is my favourite OSS project.

	David Baron :dbaron: Assignee
	Comment 15 • 18 years ago

I checked in the current versions of the XUL harness into layout/tools/reftest/.

	David Baron :dbaron: Assignee
	Comment 16 • 18 years ago

(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...

	Rob Campbell [:rc] (:robcee)
	Comment 17 • 18 years ago

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?

	Adam Guthrie
	Comment 18 • 18 years ago

Rob, see http://lxr.mozilla.org/seamonkey/source/layout/tools/reftest/README.txt.

	David Baron :dbaron: Assignee
	Comment 19 • 18 years ago

./firefox -reftest <manifest>

	Rob Campbell [:rc] (:robcee)
	Comment 20 • 18 years ago

ah, thank you both. I had just found the README file.

	Dave Liebreich [:davel]
	Comment 21 • 18 years ago

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)

	Rob Campbell [:rc] (:robcee)
	Comment 22 • 18 years ago

(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!

	Dave Liebreich [:davel]
	Comment 23 • 18 years ago

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

	David Baron :dbaron: Assignee
	Comment 24 • 18 years ago

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.

	David Baron :dbaron: Assignee
	Comment 25 • 18 years ago

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.

	Dave Liebreich [:davel]
	Comment 26 • 18 years ago

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

	Dave Liebreich [:davel]
	Comment 27 • 18 years ago

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

	David Baron :dbaron: Assignee
	Comment 29 • 18 years ago

Why?  Nothing references that makefile yet, and shouldn't until it works.

	Dave Liebreich [:davel]
	Comment 30 • 18 years ago

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.

	David Baron :dbaron: Assignee
	Comment 31 • 18 years ago

So I definitely want this to be part of something once it works.  What should that be?  There is no recursive |make lcheck| target.

	Dave Liebreich [:davel]
	Comment 32 • 18 years ago

(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.

	Dave Liebreich [:davel]
	Comment 33 • 18 years ago

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.

	Dave Liebreich [:davel]
	Comment 34 • 18 years ago

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.

	David Baron :dbaron: Assignee
	Comment 35 • 18 years ago

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"

	Dave Liebreich [:davel]
	Comment 36 • 18 years ago

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.

	Dave Liebreich [:davel]
	Comment 37 • 18 years ago

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

	Dave Liebreich [:davel]
	Comment 38 • 18 years ago

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.

	Dave Liebreich [:davel]
	Comment 39 • 18 years ago

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

	David Baron :dbaron: Assignee
	Comment 40 • 18 years ago

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.

	Nickolay_Ponomarev
	Comment 41 • 18 years ago

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?