search

jejacks0n / teaspoon

Teaspoon: Javascript test runner for Rails. Use Selenium, BrowserStack, or PhantomJS.
1.43k stars 244 forks source link

Use Open3.capture2e instead of %x{} and $? #476

Closed baburdick closed 6 years ago

baburdick commented 8 years ago

note: this fixes text + text-summary coverage reports (previously generated only text report when both were config'd -- and the spec enshrined this)

note: using $? with %x{} can lead to missing exit status code issues

ref: modeset/teaspoon#475

baburdick commented 8 years ago

Fixing hound-bot critiques ...

baburdick commented 8 years ago

@jejacks0n: any thoughts on this PR?

jejacks0n commented 8 years ago

It's looking good -- thanks for the time you've put into it. Can you provide a link so I understand why $? with %x{} can lead to missing exit status code issues? Just want to learn from it. Also, in a few other places I use IO.popen -- is there a reason not to do the same here? Again, mostly just trying to figure out the subtle differences.

baburdick commented 8 years ago

I wish I could provide a link. And I wish I understood better what's at the core of the problem. But it's a conclusion I've drawn from this experience. On my machine, using pry, I was able to witness this issue, described in modeset/teaspoon#475 .

Using this solution, which returns STDOUT, STDERR, and the exit status code in a single array, I can't reproduce the issue.

With respect to IO.popen, I see that you're using it in these two files, and depending on Ruby on subsequent lines to put $? into the local scope:

If IO.popen uses the same mechanism to put $? into the local scope, the same problem could manifest. But I haven't seen it in my environment. And so far I haven't seen anyone else report a similar issue (although my Google-foo may have failed me in that regard).

Using the standard library's Open3 and one of its capture* methods to return the subset of STDOUT, STDERR, and exit status code that you need definitely eliminates depending on Ruby's mechanism to insert the exit status code in to the local scope in $?.

The only thing I can further suggest is to take a look at my change to the "generates coverage reports" spec. If you agree that it is now testing for the proper output, given the inputs, then something is not working properly in master. But I'm not certain the cause is what's described above. My changes expose more of the implementation to testability. So that fix could just be a side-effect of the better testability.

I wish I had an open-and-shut case that you could reproduce, or could point definitively to an existing bug in Ruby. But I have to depend on your judgement here. I hope my reasoning is clear and sensible to you.

baburdick commented 8 years ago

Checking back in. It looks like Open3 provides alternatives to IO.popen: Open3.popen2, Open3.popen2e, and Open3.popen3. For your use of IO.popen and $?, it looks like you could substitute Open3.popen2e.

Also, here's a few links providing more insight into what the various forms of process spawning do and how and why:

baburdick commented 8 years ago

@jejacks0n: You may have missed my last post here. Please take a look. It may satisfy your curiosity.

ivdma commented 7 years ago

@jejacks0n this still isn't merged :(

patrickmclaren commented 7 years ago

@jejacks0n Anything left for this to merge?

baburdick commented 7 years ago

@jejacks0n: Just checking in. Completely understood if you're very busy. Looking forward to when you can check this out and let me know if there's more you need from me. (Happy New Year, BTW.)

baburdick commented 6 years ago

Not sure what's breaking the build. It appears that merging in the most recent master and pushing caused current Travis to expose issues in most recent yet old master.

baburdick commented 6 years ago

@jejacks0n: Looks like jejacks0n/teaspoon#534 fixes all the breakage here. Let me know what I can do further here.

baburdick commented 6 years ago

Thank you, @jejacks0n!

baburdick commented 6 years ago

Verified that this works. All that remains is to build and push to rubygems.