This is an archive of the discontinued LLVM Phabricator instance.

[TestEvents] Add a 'connected' state to include remote debugging.
ClosedPublic

Authored by sivachandra on May 7 2015, 3:02 PM.

Diff Detail

Event Timeline

sivachandra updated this revision to Diff 25247.May 7 2015, 3:02 PM
sivachandra retitled this revision from to [TestEvents] By pass 'connected' state for remote debugging..
sivachandra updated this object.
sivachandra edited the test plan for this revision. (Show Details)
sivachandra added reviewers: chaoren, vharron.
sivachandra added a subscriber: Unknown Object (MLST).
chaoren added inline comments.May 7 2015, 3:46 PM
test/python_api/event/TestEvents.py
249–250

Update comment here?

259

Update comment here?

276–277

Perhaps we need another self.context.state value for this to ensure ordering (Bump everything after by 1).

279

s/pass/continue/?

294

We don't need this.

sivachandra added inline comments.May 7 2015, 4:03 PM
test/python_api/event/TestEvents.py
249–250

See my comment at 275.

259

See my comment at 275.

276–277

Should we be treating this as a state at all? The 'connected' event is just a notification of being connected and has nothing to do with the inferior state.

279

Re-reading, 'continue' seems correct. Let me know your opinion around other changes and will change this to continue along with them.

294

Perhaps. But whoever has put it, probably wanted to see a message when listener.WaitForEvent times out. That's my guess, which if correct should be have been under an else.

We could ofcourse remove this altogether as I think it was a debug print statement which the committer forgot to remove before committing.

chaoren added inline comments.May 7 2015, 4:13 PM
test/python_api/event/TestEvents.py
276–277

I think state describes this test (which event to expect next), and not the inferior. Regardless, It's still nice to ensure ordering.

294

Is it an error condition if all 6 attempts fail? Perhaps we should just remove this line and error out below.

297

Error here?

sivachandra added inline comments.May 7 2015, 4:53 PM
test/python_api/event/TestEvents.py
276–277

If ordering is a concern, how about this:

if match.group(1) == 'connected':
  self.assertTrue(self.context.state == 0)
  continue

If 'connected' is treated as a state, then there will be a different state machine (a different start state) for remote and non-remote platforms. It sounds unnecessarily complicated, but will do it if you really want it.

294

That makes it out of scope for this change. I am going to remove all my changes around this to avoid further mixup.

chaoren added inline comments.May 7 2015, 5:02 PM
test/python_api/event/TestEvents.py
276–277

Why not store the previous state as a string?

if match.group(1) == 'connected':
  self.assertTrue(self.context.state == None)
  self.context.state = 'connected'
  continue
elif match.group(1) == 'running':
  self.assertTrue(self.context.state == None or self.context.state == 'connected')
  self.context.state = 'running'
  continue
...
sivachandra updated this revision to Diff 25270.May 7 2015, 5:35 PM

Reaching round my neck...

sivachandra retitled this revision from [TestEvents] By pass 'connected' state for remote debugging. to [TestEvents] Add a 'connected' state to include remote debugging..May 7 2015, 5:37 PM
chaoren accepted this revision.May 7 2015, 5:38 PM
chaoren edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 7 2015, 5:38 PM
sivachandra closed this revision.May 7 2015, 5:46 PM