Page MenuHomePhabricator

Test case for the issue raised in D32271

Authored by vbalu on Apr 26 2017, 12:16 AM.

Diff Detail


Event Timeline

vbalu created this revision.Apr 26 2017, 12:16 AM
vbalu edited the summary of this revision. (Show Details)
labath added a subscriber: labath.Apr 26 2017, 2:15 AM

Thank you for taking the time to add the test.

I'd like to avoid modifying the test framework for the sake of this test, if that is possible.

1487 ↗(On Diff #96676)

How about you just modify the test to create the directory and copy the file there? That way you will not have to modify the general test framework for a one-off test...

vbalu updated this revision to Diff 96878.Apr 27 2017, 12:15 AM

Removed the changes from test frame work.

labath accepted this revision.Apr 27 2017, 3:10 AM

Cool. Thank you.

This revision is now accepted and ready to land.Apr 27 2017, 3:10 AM
emaste added inline comments.Apr 30 2017, 12:36 PM
43 ↗(On Diff #96878)

whitespace issue here in raw diff

emaste requested changes to this revision.Apr 30 2017, 1:01 PM

This test passes even without the change in D32271, presumably because exe = os.path.join(os.getcwd(), 'newdir/proc_attach') is providing us with an absolute path for argv[0].

With a change like that below the test passes with D32271 and fails without it:

# Spawn a new process
popen = self.spawnSubprocess('./proc_attach')
This revision now requires changes to proceed.Apr 30 2017, 1:01 PM
vbalu updated this revision to Diff 97550.May 2 2017, 11:10 PM
vbalu edited edge metadata.

Corrected the code. Now this test will fail without D32271 patch.

vbalu updated this revision to Diff 97575.May 3 2017, 2:31 AM
vbalu marked an inline comment as done.

modified exe path computation

Two small nits noted inline, but now the test does not pass without D32271 and does with it. Note that in the "without" case it returned error rather than failing, perhaps related to the path issue I noted?

Other than that issue I think this is good to go. So hopefully that can be addressed, then one of the Linux folks can confirm it's good to go. IMO it's fine for this to go in first demonstrating the bug on FreeBSD followed by a commit of D32271.

42 ↗(On Diff #97575)

s/frm/from/ perhaps?

47 ↗(On Diff #97575)

shouldn't this be ... rmtree(os.path.join(os.getcwd(), 'newdir')) instead?

labath added inline comments.May 22 2017, 7:01 AM
47 ↗(On Diff #97575)

Probably not because he does a os.chdir(newdir) below. However, that does not make this a good idea, as you will delete an unexpected directory if the test fails before you get a chance to chdir. I like the Ed's earlier suggestion to launch the process from a separate directory more than this.

vbalu updated this revision to Diff 100579.May 28 2017, 10:36 PM

Modified as suggested. sorry for the delay, i caught up in other work. Please let me know if something goes wrong.

Generally LGTM with two questions inline.

45 ↗(On Diff #100579)

Does this one need to use os.path.join too?

53 ↗(On Diff #100579)

Does the test infra automatically start each test in the correct directory? (I.e., chdir back to the original directory is not required?)

Ping for two open questions

vbalu updated this revision to Diff 113067.Aug 29 2017, 5:15 AM
vbalu marked 2 inline comments as done.

done changes as ssuggested

Testing this just now I got:

FAIL: LLDB (/usr/bin/cc-x86_64) :: test_attach_to_process_from_different_dir_by_id (TestProcessAttach.ProcessAttachTestCase)
ERROR: test_attach_to_process_from_different_dir_by_id (TestProcessAttach.ProcessAttachTestCase)
   Test attach by process id                                                    
Traceback (most recent call last):                                              
  File "/tank/emaste/src/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/process_attach/", line 44, in test_attach_to_process_from_different_dir_by_id
OSError: [Errno 17] File exists: '/tank/emaste/src/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/process_attach/newdir'


emaste added inline comments.Sep 2 2017, 2:33 PM
44 ↗(On Diff #113067)


except OSError, e:                                                     
    if e.errno != os.errno.EEXIST:                                     
This revision was automatically updated to reflect the committed changes.