Bugzilla – Full Text Bug Listing
| Summary: | [tests] Eliminate the 1s wait at start of each DSF-GDB test. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Pawel Piech <pawel.1.piech> | ||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | John Cortell <john.cortell> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||
| Severity: | minor | ||||||||
| Priority: | P3 | CC: | john.cortell, pawel.1.piech | ||||||
| Version: | 7.0 | Flags: | john.cortell:
review?
(pawel.1.piech) |
||||||
| Target Milestone: | 7.0 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Pawel Piech
(In reply to comment #0) > The code in BaseTestCase.baseBeforeMethod() can potentially wait an extract 1 > sec. for each launch, depending on system timing. This can unnecessarily delay > the overall test run. There's a way to avoid this delay, see the attachment in > bug 202551 c#2. I like the pattern of using the DsfSession.addSessionStartedListener. But I'm curious as to when BaseTestCase.baseBeforeMethod() would wait an extra 1 second. We should always get an MIStopped event, so we will never wait the full second, right? I would _really_ like to increase the speed of the JUnit tests. Right now, they take so much time that it is annoying enough that I don't run them as often as I should... Bad me. Speeding up the tests would be great. But what we need above all else, IMO, are these tests running every night on the build machine. The fact that individual engineers have to run these tests on their own initiative is problematic on many levels. Worrying about their performance seems like a misdirection of focus. One could argue that such performance *mostly* becomes irrelevant when these tests are run on a build machine at 3AM, e.g. I'm not saying it's something we shouldn't bother with, but if we're going to spend cycles that are in short supply, I'd prefer to see them spent on getting the tests run in an automated fashion. That stone will kill many more birds. (In reply to comment #2) > getting the tests run in an > automated fashion. That stone will kill many more birds. +1 More dead birds is better :-O (In reply to comment #1) > But I'm curious as to when BaseTestCase.baseBeforeMethod() would wait an extra > 1 second. We should always get an MIStopped event, so we will never wait the > full second, right? In my testing with the PDA debugger, I noticed that the started event (which I was waiting for in that case) was always being sent before the lc.launch() returned. So the wait was always triggered. You can set a breakpoint in the catch() statement and see if you hit the timeout exception. (In reply to comment #1) > But I'm curious as to when BaseTestCase.baseBeforeMethod() would wait an extra > 1 second. We should always get an MIStopped event, so we will never wait the > full second, right? Marc, in troubleshooting a run-control test failure on Windows, I've stumbled onto something about this mechanism that I think is deficient. We delay the start of a test until the MIStopped event arrives. I think that's insufficient. We should wait until a higher-level ISuspendedDMEvent is received, otherwise a test can begin before the session is fully settled in the suspended state. This can be easily proven with the following test @Test public void runToLine() throws Throwable { // The following should fail but it doesn't // because the ISuspendedEvent hasn't arrived yet ServiceEventWaitor<ISuspendedDMEvent> waitor = new ServiceEventWaitor<ISuspendedDMEvent>( getGDBLaunch().getSession(), ISuspendedDMEvent.class); waitor.waitForEvent(1000); } (In reply to comment #5) > (In reply to comment #1) > > But I'm curious as to when BaseTestCase.baseBeforeMethod() would wait an extra > > 1 second. We should always get an MIStopped event, so we will never wait the > > full second, right? > Marc, in troubleshooting a run-control test failure on Windows, I've stumbled > onto something about this mechanism that I think is deficient. We delay the > start of a test until the MIStopped event arrives. I think that's insufficient. > We should wait until a higher-level ISuspendedDMEvent is received, Back in the day, I didn't understand the difference between all those events. ISuspended makes sense now. Another issue. That code acts like it's no big deal if we miss the stopped event, but it's actually a very big deal. There are numerous tests that use the context of that stopped event. So, I really don't see the point of ignoring the timeout failure there. It just means we have tests that will intermittently fail, and the reason won't be terribly obvious. There are ways to mitigate this (to make the failure more obvious), but I just don't like this uncertainty... Shouldn't there be a way to listen for service events at the outset of a session without the possibility of missing events? Ooof. I further noticed that BaseTestCase.baseBeforeMethod is creating a ServiceEventWaitor on a thread other than the DSF executor. That's not good. I think we need to maybe go to rethink how delay the start of a test method until the target is suspended. In English... I recommend we rethink how we delay the start of a test method until the target is suspended. (In reply to comment #7) > Shouldn't there be a way to listen for service events at the outset of a > session without the possibility of missing events? In the VM test that I'm working on I add the listener in the session started listener which should guarantee that the listener is added in the session thread and that we don't miss any events. (In reply to comment #10) > (In reply to comment #7) > > Shouldn't there be a way to listen for service events at the outset of a > > session without the possibility of missing events? > In the VM test that I'm working on I add the listener in the session started > listener which should guarantee that the listener is added in the session > thread and that we don't miss any events. Nice. I'll give it a shot. Thanks! (In reply to comment #8) > Ooof. I further noticed that BaseTestCase.baseBeforeMethod is creating a > ServiceEventWaitor on a thread other than the DSF executor. That's not good. I > think we need to maybe go to rethink how delay the start of a test method until > the target is suspended. More suff I hadn't quite figured out back then. See bug 292263 about more. Created attachment 170272 [details]
Fix`
Fix. I actually bumped up the wait to two seconds, but there is now no possibility of missing the stopped event. Thus a test will now fail if we don't receive the event. The end result is that there is no longer a potential for a needless one second wait.
Committed to HEAD. Pawel, please review since you implemented a similar mechanism (I imagine; I didn't actually look at your patch in 202551) BTW, this patch also addresses the issue I raised in comment # 5. The test will not proceed until the higher-level ISuspendedEvent is received. Found a problem with fix. Adjusting... Created attachment 170281 [details]
Update to fix
The base test class shouldn't register and deregister itself as a session event listener, as the derivatives are likely to do that.
*** cdt cvs genie on behalf of jcortell *** Bug 314174: Eliminate the 1s wait at start of each DSF-GDB test. [*] BaseTestCase.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/framework/BaseTestCase.java?root=Tools_Project&r1=1.10&r2=1.11 [*] BaseTestCase.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/framework/BaseTestCase.java?root=Tools_Project&r1=1.11&r2=1.12 *** cdt cvs genie on behalf of jcortell *** Bug 314174: Eliminate the 1s wait at start of each DSF-GDB test. (Avoid possible NPE when a test fails) [*] BaseTestCase.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/framework/BaseTestCase.java?root=Tools_Project&r1=1.12&r2=1.13 |