Re: [webkit-dev] DRT/WTR should clear the cache at the beginning of each test?
We can live in one of two worlds: 1) LayoutTests that concern themselves with specific network/loading concerns need to use unique URLs to refer to static data; or 2) DRT clears JS-visible state between tests. The pros/cons seem clear to me: Pro#1: loading/caching code is coincidentally tested by (unknown) tests that reuse URLs among themselves. Con#1: requires additional cognitive load for all webkit developers; the only way to write a test that won't be affected by future addition of unrelated tests is to use unique URLs Pro#2: principle of least-surprise is maintained; understanding DRT reading a test (and not every other test) is enough to understand its behavior Con#2: loading/caching code needs to be tested explicitly. IMO (Pro#2 + -Con#1) (Pro#1 + -Con#2). Are you saying you believe the inequality goes a different way, or am I missing some other feature of your thesis? Yes, this is a fair description. I'm going to assume you mean that yes, you believe the inequality goes the other way: (Pro#2 + -Con#1) (Pro#1 + -Con#2) This accidental testing is not something to be neglected I'm not neglecting it, I'm evaluating its benefit to be less than its cost. To make concrete the cost/benefit tradeoff, would you add a random sleep() into DRT execution to detect timing-related bugs? It seems like a crazy thing to do, to me, but it would certainly catch timing-related bugs quite effectively. If you don't think we should do that, can you describe how you're evaluating cost/benefit in each of the cases and why you arrive at different conclusions? (of course, adding such random sleeps under default-disabled flag control for bug investigation could make a lot of sense; but here I'm talking about what we do on the bots by default) It's not humanly possible to have tests for everything in advance. Of course. But we should at least make it humanly possible to understand our tests as written :) Making understanding our tests not humanly possible isn't the way to make up for the not-humanly-possible nature of testing everything in every way. It just means we push off not knowing how much coverage we really have, and derive a false sense of security from the fact that bugs have been found in the past. I completely agree with Maciej's idea that we should think about ways to make non-deterministic failures easier to work with, so that they would lead to discovering the root cause more directly, and without the costs currently associated with it. I have no problem with that, but I'm not sure how it relates to this thread unless one takes an XOR approach, in which case I guess I have low faith that the bigger problem Maciej highlights will be solved in a reasonable timeframe (weeks/months). Memory allocator state. Computer's real time clock. Hard drive's head position if you have a spinning hard drive, or SSD controller state if you have an SSD. HTTP cookies. Should I continue the list? These things are all outside of webkit. Yes, they are outside WebKit, but not outside WebKit control, if needed. Did you intend that to be an objection? I imagine Balazs was pointing out that you included items that are not JS-visible in an answer to my question about things that are JS-visible. But that was part of an earlier fork of this thread that went nowhere, so let's let it go. Cheers, -a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] DRT/WTR should clear the cache at the beginning of each test?
There are lot of things remaining in the process across tests runs What things remain in the process across test runs that are visible to DRT/JS? As I've said before in this thread, it seems axiomatic to me that tests can only be reasoned about if they run in a pristine environment. This is why we TestShell::resetTestController()http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/TestShell.cpp#L300; so that a given test passes or fails the same way regardless of what other tests have run in the same process earlier. Given that we *do* reset the execution environment between tests, it seems arbitrary (and unworkable) to *not* reset the cache. Cheers, -a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] DRT/WTR should clear the cache at the beginning of each test?
This thread stalled out because although there seemed to be majority agreement that hermetic/repeatable tests are a good thing, there was a requirement that all ports be updated to the new behavior at the same time, and I'm only competent to do the chromium DRT (see https://bugs.webkit.org/show_bug.cgi?id=93195 for details). Is anyone interested in stepping up and doing the equivalent (clear caches between tests) for the mac and/or gtk ports' DRTs? On Wed, Aug 8, 2012 at 2:35 PM, Dirk Pranke dpra...@chromium.org wrote: On Wed, Aug 8, 2012 at 10:47 AM, Ojan Vafai o...@chromium.org wrote: See https://bugs.webkit.org/show_bug.cgi?id=93195. media/W3C/video/networkState/networkState_during_progress.html and media/video-poster-blocked-by-willsendrequest.html are flaky on all platforms because they behave differently if the loaded resource is cached. Every time I've taken a stab at reducing test flakiness, I've come across at least a few tests that pass when run as part of the test suite, but fail when run by themselves (or in parallel) because they accidentally expect an image or something to be in the cache. I think it would make the tests more maintainable if we cleared the cache before each test run. This is *not* before each page load though. So tests that do multiple page loads will still test cross-navigation caching behavior. While it's true that we could one-off fix each of these tests, it's usually very time consuming to figure out that caching is the problem, that's assuming anyone takes the time to look into why the test is flaky in the first place. Any objections? Given that the way we run tests in parallel in NRWT means that different processes get different lists of tests each time, it sounds like we may be getting a fair amount of nondeterminism from the cache not being cleared between tests. That seems bad, so I'm in favor of clearing the cache :) -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] DRT/WTR should clear the cache at the beginning of each test?
On Fri, Oct 26, 2012 at 1:44 AM, Antti Koivisto koivi...@iki.fi wrote: Cache has subtle interactions with other things being tested (-flakiness). More explicit cache tests would be nice but we can't hope the replicate all the accidental testing we now get. We are going to lose a large chunk of existing test coverage if we do this. The reality is that this test coverage today shows up as flakiness and so is ignored anyway, meaning we don't actually have useful coverage here. Even when flakiness is investigated, the fix is to cache-bust using unique URL params, which just means we lose the coverage you describe for that test, anyway. Brian notes in the bug that GTK wk2 GTK+ are done. I believe that just leaves chromium mac. Anyone wanting to step up to do mac, and, I guess, wk2 mac? -a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] DRT/WTR should clear the cache at the beginning of each test?
On Fri, Oct 26, 2012 at 11:17 AM, Ryosuke Niwa rn...@webkit.org wrote: I agree this is a good change but it appears that we should add more cache/loader tests before changing DRT's behavior given that there are active contributors who rely on the current DRT behaviors to detect regressions. Not knowing the specifics of the regressions in question, I don't have any idea what these new cache-related tests would be. -a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] DRT/WTR should clear the cache at the beginning of each test?
Should we add random sleeps to DRT? It'll certainly help find some regressions (and even security bugs). Of course the down-side is that it makes tests non-repeatable and difficult to reason about. I'm baffled by your priorities and don't know how to continue this conversation productively. Sorry. Cheers, -a On Fri, Oct 26, 2012 at 12:43 PM, Alexey Proskuryakov a...@webkit.org wrote: 26.10.2012, в 11:04, Antti Koivisto koivi...@iki.fi написал(а): The reality is that this test coverage today shows up as flakiness and so is ignored anyway, meaning we don't actually have useful coverage here. Even when flakiness is investigated, the fix is to cache-bust using unique URL params, which just means we lose the coverage you describe for that test, anyway. I think that this is the real issue here. Test flakiness is very important to investigate, this often leads to discovery of bad bugs, including security ones. The phrase flaky test often misplaces the blame. When making cache related changes I have frequently found bugs from my patches because some seemingly random test started failing and I investigated. Without the test coverage some of those bugs would probably now be in the tree. I agree with Antti. Finding regressions is what tests are for, and it would be difficult to make enough explicit tests to compensate for such loss of coverage. It would certainly be very unfortunate to lose test coverage without even an attempt to compensate for that. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] DRT/WTR should clear the cache at the beginning of each test?
On Fri, Oct 26, 2012 at 2:11 PM, Ryosuke Niwa rn...@webkit.org wrote: Is there any reason we can’t wait for another couple of weeks or months until we add more loader cache tests before making the behavior change? There is no time pressure here other than a desire to avoid this falling between the cracks and (continuing to) never being done. Is anyone signing up to write or enumerate the tests, who can do the work in the next weeks/months, but not immediately? -a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] A plug for the Watch List
Bugzilla sends a notification email when one is add to the CC list of a bug. IWBN if that email would include the state of the bug so far. My experience with a few weeks' worth of being on a watchlist is that the resulting email threads tend to miss the most important information of the why/what/where context usually included in the original bug report, but missing from the email thread because I'm only cc'd once a patch is uploaded. Cheers, -a On Wed, Apr 25, 2012 at 11:33 AM, Eric Seidel e...@webkit.org wrote: As explained here: http://trac.webkit.org/wiki/WatchList The style-bot applies the watchlist regexps when it checks a patch, and will CC you at that time. On Wed, Apr 25, 2012 at 11:26 AM, Eric Seidel e...@webkit.org wrote: Are you an expert on FOO? Does your port need to know if BAR changes? Are you sick and tired of those insert company name here people breaking your favorite BAZ!?!? Add yourself to the watchlist: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist Especially if you're a good reviewer for a specific file. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] exporting symbols for building .so/.dll's
I'm inclined to give this thread one more business day and then call it
Tuesday morning pacific time.
On current showing I think the approach in the patch on the bug (incl.
morrita@'s preference for WEBCORE_EXPORT_PRIVATE) is the way to go, at
least for now.
If you have strong concerns please speak up before then.
Cheers,
-a
On Sun, Mar 11, 2012 at 3:02 AM, Hajime Morrita morr...@chromium.orgwrote:
(From the right address again...)
On Sun, Mar 11, 2012 at 9:34 AM, Alp Toker a...@nuanti.com wrote:
On 09/03/2012 03:52, Ami Fischman wrote:
Hi webkittens,
The over-all question: how should webkit libraries declare which symbols
they export?
The trigger for the question: as described in bug 80062, the chromium
shared-library-based build links test code into the (non-test)
libwebkit.so
target, which is terrible.
The details:
I took a crack at fixing the above bug (see patch in the bug) by pulling
the
test files out of the non-test build target, and sprinkling various
{WTF,WEBKIT}_EXPORT{,DATA} macros around declarations that need them (as
discovered by build-time and run-time failures). This style of export
declaration is what the webkit codebase does in various places
(WTF, Platform, WebCore, and WebKit, AFAICT), and incidentally also what
the chromium project uses for its sub-components.
But I'm told other ports use different mechanisms such as .exp.infiles for
apple/mac (and maybe others for other ports? IDK).
Is there consensus on the list for what the Right Thing To Do(tm) is?
ISTM my options are:
1) Add EXPORT declarations as in the patch on the bug.
2) Drop the patch from the bug and replace it with chromium-specific
.exp.in-style files, one per layer from which I need to export (WebCore,
WTF, WebKit). And build the build-time machinery to use them. I don't
really know what's involved here and would appreciate any pointers/hints
people had if this is the way to go.
3) Something else (preferably unifying other ports' approaches).
Help me webkit-dev, you're my only hope (for consensus).
I think the export macros would only ever have made sense if they were
put
there explicitly to guide refactoring of the classes into a library /
interface structure. And this isn't the case.
At present I don't see an active effort towards that, or much interest in
defining the public interfaces in each 'module' more strictly. They're
intentionally fluid.
Having said that, the macros are /vaguely/ useful to see what could be
made
private or hidden in future shuffling of the code in wtf, for example,
but
that's about it.
The very fact that the export macros have to be updated with a tool every
time a library higher in the link chain uses or doesn't use a public
entry
point, and that the set of imported functions or variables varies between
ports indicates that this is not going to have wide adoption.
This is same for port specific export lists.
To keep the tree green, the community needs to maintain the list
regardless it is externalized from or embedded in the source.
And maintaining the set of export lists has larger pain than
maintaining a macro-based annotation.
Thus keeping it isolated won't help us much.
, and that the set of imported functions or variables varies between
ports indicates that this is not going to have wide adoption.
As mrobinson@ mentioned, there is a set of the least common denominator.
My guess is that the number of symbols which is different between
ports will be less than a half of the total.
If we accept such overhead, it could be a preferable option.
If we follow this to the logical conclusion, no unification of granular
export lists is realistic with the current WebKit porting layer. If the
strategy were adapted to define exported functionality at class
granularity,
it might just be feasible, but again that is a contract that is begging
to
be broken, and besides, most toolchains lack export-by-class so it's a
moot
point.
At least recent gcc+ld and link.exe support such a class level annotation.
For GNU chain, the lack of support seems like an old story:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=9283
My feeling is that the macro annotation is never for declaring a public
API.
It's just a pretty convenient replacement of the port specific export
lists, with small overhead from possible unwanted exports.
It's different from WebKit API which each port is providing.
There is no commitment for compatibility, as the _PRIVATE suffix
indicates.
To clarify: I think that there should be WEBCORE_EXPORT_PRIVATE but no
WEBCORE_EXORT.
This is unlike JSC, which has both JS_EXPORT and JS_EXPORT_PRIVATE.
(I prefer some shorter name btw... but it's a different topic.)
So the ultimate course of action is then to revert the macros and leave
everyone to do what they think best in terms of export lists, then tying
together those solutions where
Re: [webkit-dev] exporting symbols for building .so/.dll's
Responding to several emails below. Adam wrote: I'm not sure I understand your proposal fully. Specifically, how would these macros work for, say, the Chromium, Apple-Mac, and Apple-Win ports, which export overlapping, but not identical, sets of symbols? I do not have a proposal to unify the export story across ports. My proposal (such as it is) is limited to removing test code from the non-test webkit target (see the patch on the bug). AFAICT from this thread there is no consensus on unifying options that doesn't have as its first step achieve consensus on the complete public API of each library layer, and I didn't get the sense that this is a realistic goal, at least for me at my current level of engagement. Adam also wrote: Relatedly, do you plan to replace http://trac.webkit.org/browser/trunk/Source/WebCore/WebCore.exp.in with EXPORT macros, or will we have both macros and an export list for an extended period of time? The latter. To put things in perspective, WebCore.exp.in is over 2000 lines long; my patch macro-annotates ~40 classes in WebCore (which is a far cry from, say, doubling the pain of WebCore.exp.in). I'm also emboldened in this respect by the coexistence of WEBKIT_EXPORT and Source/WebKit/mac/WebKit.exphttp://code.google.com/p/chromium/source/search?q=file%3AWebKit%2FSource%2FWebKit%2Fmac%2FWebKit.exporigq=file%3AWebKit%2FSource%2FWebKit%2Fmac%2FWebKit.expbtnG=Search+Trunk . Alp wrote: Doesn't the list of exports change massively based on the level of inlining and compiler optimisation? Which configuration is the target to aim for, and which compiler vendor? Doesn't the list also change between releases of the same compiler? AFAICT the answers to your questions are all no, for the limited scope of my change (some indication of this is that I only tested my patch on chromium/linux, but it's now green on all EWS bots after only needing to add #include's to get the definition of the export macro, not due to having to add extra annotations for some ports). As with the existing JSC_EXPORT and WEBKIT_EXPORT macros, the new WEBCORE_EXPORT_PRIVATE (nee (unused) WEBKIT_EXPORTDATA) will be an annotation added by whoever lands a patch that requires access to something from outside the library that today doesn't require it. Ash proposed replacing both the explicit post-mangled export lists and in-line macros with per-port export-lists that would be used at build time to emit modified headers. I'd have to see a PoC of this to believe that it wouldn't combine the worst aspects of all the options on the table ;) The proposal assumes a lot of things I don't believe are *strictly* true, and I believe the gap between almost right and strictly right is where enough pain will come from to sink this option. Cheers, -a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] exporting symbols for building .so/.dll's
Hajime I chatted and it became clear that we've been miscommunicating. I'm taking that conversation off webkit-dev and putting this thread on hold pending that resolving. Sorry for the noise. Cheers, -a On Sun, Mar 11, 2012 at 5:54 PM, Hajime Morrita morr...@chromium.orgwrote: I found some confusion here. There are two kinds of symbols which need to be exported from WebCore. - A. Symbols from WebCore to WebKit(API). This is basically what WebCore.exp.in covers. - B. Symbols from WebCore to Application. In principle, there should be no such symbols. But in reality there is some. Symbols for Internals objects follow this pattern. Note that the number of B is significantly smaller than one of A. It looks Alp's main concern is about B. But I implicitly assumed A. Currently A matters only for Mac WK1 port (Webcore.exp.in). Some other ports including GTK, WebKit2 needs B. (symbols.fiter, WebKit_Cairo.def, etc.) Mac WK1 also needs B. But it's in the same WebCore.exp.in list. For A, it should be marked as WEBCORE_EXPORT_PRIVATE if we use inline annotations. For B, it should use WEBCORE_EXPORT. For A, I think it's infeasible to replace it with inline annotations because the .exp list and inline annotations are mutually exclusive. So we need to make it happen at once. But it's not a simple job. I think we need some incremental transition plan. It might be someting like what Alp mentioned, that is, to use nm or whatever to automate exp list generation, etc. For B, it's relatively easy to turn the macro on because its number is small. The symbols.filter file shows about 100 symbols, which would be manually convertible. What's tricky is that Chromium component build wants yet another kind of symbols: The symbols is for unit testing. Let me call it C otpion here. C is different from either A or B. It touches deep WebCore internals which people don't need to export unless they write unit tests, that is current WebKit situation. So, - Some ports need A+B (ex. Mac WK1 Port) - Some ports need B (ex. GTK, Windows) - Some ports need B+C (Chromium) And my feeling is that Chromium is OK to have A if it's available. Having that said, here is my proposal: - Let's introduce WEBCORE_EXPORT for B. Chromium will use this, GTK and Windows also could use. (For GTK, I expect this to have same definition as WEBKIT_API.) - Let's NOT introduce WEBCORE_EXPORT_PRIVATE for now because it would be useless unless we don't get rid of WebCore.exp.in, which won't easy. - Let's introduce WEBCORE_EXPORT_TEST for C. Chromium will enable and use this for now. - If WEBCORE_EXPORT and WEBCORE_EXPORT_TEST conflicts, WEBCORE_EXPORT win. - If WEBCORE_EXPORT_PRIVATE and WEBCORE_EXPORT_TEST conflicts, WEBCORE_EXPORT_TEST win. - If WEBCORE_EXPORT and WEBCORE_EXPORT_PRIVATE conflicts (in the future), WEBCORE_EXPORT win. Then we can start from adding C for Chromium, which doesn't have any export list before. Chromium will also need B. Having B would be good news also for GTK and Windows port because these no longer need to touch their symbols file for each time the Internals object needs one. I'm happy to help this to happen. (... and I'd like to ask Ami to help ;-)) What do you think? -- morrita On Mon, Mar 12, 2012 at 9:16 AM, Ami Fischman fisch...@chromium.org wrote: Responding to several emails below. Adam wrote: I'm not sure I understand your proposal fully. Specifically, how would these macros work for, say, the Chromium, Apple-Mac, and Apple-Win ports, which export overlapping, but not identical, sets of symbols? I do not have a proposal to unify the export story across ports. My proposal (such as it is) is limited to removing test code from the non-test webkit target (see the patch on the bug). AFAICT from this thread there is no consensus on unifying options that doesn't have as its first step achieve consensus on the complete public API of each library layer, and I didn't get the sense that this is a realistic goal, at least for me at my current level of engagement. Adam also wrote: Relatedly, do you plan to replace http://trac.webkit.org/browser/trunk/Source/WebCore/WebCore.exp.in with EXPORT macros, or will we have both macros and an export list for an extended period of time? The latter. To put things in perspective, WebCore.exp.in is over 2000 lines long; my patch macro-annotates ~40 classes in WebCore (which is a far cry from, say, doubling the pain of WebCore.exp.in). I'm also emboldened in this respect by the coexistence of WEBKIT_EXPORT and Source/WebKit/mac/WebKit.exp. Alp wrote: Doesn't the list of exports change massively based on the level of inlining and compiler optimisation? Which configuration is the target to aim for, and which compiler vendor? Doesn't the list also change between releases of the same compiler? AFAICT the answers to your
Re: [webkit-dev] exporting symbols for building .so/.dll's
Thanks for your reply, Morrita! For ports on approach C, it doesn't matter which WebCore/JSC API is used from WebKit API layer because they are built in the same library. If you mean that nothing in WebCore/JSC needs to be annotated as exported then I don't think that's right, because libwebkit.so has various other libwebcore_*.a's linked into it. E.g. if I revert the change I made to JavaScriptCore/wtf/chromium/ChromiumThreading.h to *not* have ChromiumThreading be declared WTF_EXPORT, then the link of webkit_unit_tests fails because libwtf.a (JavaScriptCore/wtf/chromium/MainThreadChromium.cpp) contains a call to WTF::ChromiumThreading::callOnMainThread which is a hidden symbol in libwebkit.so (WebKit/chromium/src/ChromiumThreading.cpp). Maybe I misunderstood your point? For JSC API, which A and B folks care about, there are two ways to export it. - For Windows port, there is JavaScriptCore.def. - For Mac (and WX?) port, there are macro-based annotations like JS_EXPORT, WTF_EXPORT_PRIVATE. I'm in the way to eliminate JavaScriptCore.def by replacing it with these annotations, but the migration isn't finished yet. (http://wkb.ug/76257) Yay. For WebCore API, we only need to care about WebCore.exp.in because Mac port is the only one which follows A pattern. (I remember Windows port used to use this pattern. But it looks no longer the case. Maybe they changed the way when they switched to WebKit2. But my memory might be just wrong...) I'm curious whether you have plans (or know of others' plans) to replace WebCore.exp.in in favor of macros, like you did in http://webk.it/72854 for JavaScriptCore.exp. My personal preference is to introduce WebCore version of export macros like WK_EXPORT_PRIVATE. In this way, we could use it to get rid of WebCore.exp.in in the future. Isn't that what WEBKIT_EXPORTDATAhttp://cs/#chrome/src/third_party/WebKit/Source/WebCore/platform/PlatformExportMacros.hq=file:WebCore/platform/PlatformExportMacros.h%20WEBKIT_EXPORTDATAl=39 already is? (contrary to the macro's name, it lives in WebCore; perhaps it is trickier than I realize and somehow only applies to WebKit code even though it lives in WebCore? Or maybe it just needs to be renamed?) Yet another approach could be just giving up to have the unit test as a separate binary and build it into the library for non-prod configuration. The last one seems exactly what you are trying to avoid though... Right; linking test code into the main library is what HEAD does now. Anyway, I'd like to hear thoughts from other folks. Me too. As someone pointed out to me, others might be too busy with the git thread to reply to this one :) Cheers, -a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
