[Webkit-unassigned] [Bug 30504] wxPython bindings missing 'WebViewDOMElementInfo'.

Thu Oct 22 12:49:58 PDT 2009

https://bugs.webkit.org/show_bug.cgi?id=30504

--- Comment #11 from Kevin Ollivier <kevino at theolliviers.com>  2009-10-22 12:49:58 PDT ---
(In reply to comment #10)
> (In reply to comment #9)
> > In other words, no matter how I look at
> > it, I can't see how the unit testing question is appropriate for this
> > particular patch review.
> 
> Testing is always an appropriate question for a patch review.
>
> > I appreciate that sometimes r? patches stagnate for a couple weeks or more, and
> > at that time they do deserve some sort of response even if not from someone
> > explicitly knowledgeable in that area, but I rarely let a wx port r? patch sit
> > in the tree for weeks; in fact, I bet most are handled within 24 hours, so I
> > don't really understand the urge to get wx patches reviewed and out of the
> > queue when they are going to be taken care of promptly anyway by someone more
> > knowledgeable in that area.
> 
> WebKit reviewers should review any patch they feel comfortable reviewing. We
> don't have corded off areas off the code.  I felt comfortable reviewing this in
> that I speak python.  Python is very easy to unit test.  The lack of any
> attempt at testing is against normal webkit process. 
> http://webkit.org/coding/contributing.html talks about our strong belief in
> testing.

Here is what it says: "For any feature that affects the layout engine, a new
regression test must be constructed." Everything subsequent written on that
page is pretty explicitly geared towards writing layout tests. This patch does
not come anywhere near the layout engine, and thus it is not violating current
WebKit process requirements.

> > I think the wx(Python) port maintainer is probably the best person to take the
> > first crack at a completely wx(Python)-specific patch, don't you? It seems
> > almost a waste of time and effort not to, actually.
> 
> Yes, I'm glad that you review the wx patches.  You're certainly better at it
> than I am.  I still think that all patches to WebKit should have testing.  It's
> easy to test a change like this.  Creating a python unit test is dead simple:
> http://docs.python.org/library/unittest.html

I've been using Python for nearly 10 years now and I also pushed for and
mentored a SoC project to develop a PyUnit test suite for wxPython that now
runs quite a few tests (1000+ counting tests of subclass methods). I know about
unit testing in Python. You should be careful about making assumptions and
generalizations. 

My beef is that it makes little sense to argue that all changes must have unit
tests when there is no unit testing infrastructure in place in this part of the
codebase, and requesting one is not grounds for keeping this change out of the
tree, *especially* since it's not required by written WebKit policy, it's
grounds for a new ticket.

> I also don't think that just because we didn't add testing when the swig
> bindings landed doesn't mean they shouldn't have it now.

Again, file a ticket about it.

> My last statement of disagreement was that you landed this without personally
> addressing my feedback.  Not a huge deal on a patch like this, but also isn't
> the most polite thing to do.

No, but I really feel it wasn't necessary either given the situation. Moreover,
I really felt the comments about unit testing were written hurriedly and with a
lack of understanding of the wx port's current state, which I didn't find very
polite either. If you're going to take on the responsibility of a review, take
some time out to understand things first or leave it to someone who's more
familiar with things.

> I would strongly advise future changes to python bindings like this to have
> some form of unit testing. We don't have complete coverage of every piece of
> JS binding code, but every time we change the JS bindings we add a test for
> them.  Likewise, whenever the python bindings are changed, we should add tests
> for them.

Yes, but it's really easy to make statements like this when you have no idea of
the practical implications of what you're suggesting. What I need is not a pep
talk about the importance of unit testing (you're preaching to the choir), I
need people helping to get DRT running and passing all the relevant tests, I
need someone to help build a CPPUnit (or something else, if better) test
framework for the C++ API, and then when those are done, I'd love to have tests
for the bindings too. Or, I could do those things myself if someone's willing
to help fill the wx feature holes I need filled to do a release of my app for
me. :)

If you've got some ideas for how to help me actually tackle those problems,
that's something I'd really like to hear.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.