scarpe-team / scarpe

Scarpe - shoes but running on webview
Other
162 stars 29 forks source link

Add ProcessHelpers to Scarpe-Components #552

Closed noahgibbs closed 4 months ago

noahgibbs commented 4 months ago

Description

Add Scarpe::Components::ProcessHelpers#run_out_err_result. It will:

Also add tests for it. And refactor the Scarpe::Components tests a bit - only include Scarpe::Components helpers where they're needed, which makes it much easier to properly test the helpers themselves -- e.g. I initially forgot to include FileHelpers in ProcessHelpers. Just including FileHelpers into Minitest::Test, like I was doing earlier, masked that bug.

Checklist

noahgibbs commented 4 months ago

Hm. The way CI is failing here looks like another problem because GitHub Actions recently added (switched to?) ARM-based OSX runners. It's dying while trying to configure bloops. I'll look into it more.

noahgibbs commented 4 months ago

Ah, okay. I made that fix to Shoes-Spec, but not yet to Scarpe. We haven't hit it because this is the first PR we've had after the switch.

Shoes-Spec builds daily no matter what, so it hit the problem as soon as GitHub changed over.

noahgibbs commented 4 months ago

Hm. TestSSpecInfrastructure#test_timeout_test_fail was supposed to (internally) get a failure, but instead got a success. That doesn't look directly related to this change, but is clearly worth a look. Also I suspect the error will evaporate if I re-run :-/

noahgibbs commented 4 months ago

Huh. I re-ran and a different SSpec infra test (test_timeout_no_fail) got a failure. It got an unexpected skip, and the test before it is a skip test. I wonder if it's somehow not writing (or deleting, or overwriting?) the file it should?

That's possible. I did mess with that test a bit. I changed how it includes the FileHelpers module, including it into its specific test class instead of Minitest::Test. I wonder if that changed something?

That test file is passing for me locally. I wonder if it's somehow related to the ARM Mac thing? Maybe a race condition, something about processor speed? This used to pass just fine on the old GitHub Actions runners.

noahgibbs commented 4 months ago

Oh, wait, never mind. I changed how that module was included in the scarpe-components tests. But that's a different test_helper.rb and shouldn't be loaded into the same process. So no, the module include stuff should not have affected that. But the ARM Mac thing still could have.

noahgibbs commented 4 months ago

The top-level Scarpe test_helper.rb was using the same filename (local dir "sspec.json") to pass all Minitest results through. I'm worried that we were getting stale results, based on the errors I was seeing. So I switched to using a tempfile for the results -- they could be blank, but they should never be stale from previous tests. Waiting on the result of that now.

It all passes locally for me, just as it did before. But I'm trying to get more informative results from CI.

noahgibbs commented 4 months ago

Huh. Yeah, looks like there are a couple of tests in TestSSpecInfra where it's not writing a result. D'oh! And the test order was sometimes masking that, because sometimes whatever was left from last time happened to be the right thing. Well, glad that's changing now. Presumably it wasn't doing that before? But something must have happened recently, and the test failure isn't reflecting that properly.

On the plus side it should now fail every time, as of this PR. That's what the JSON errors with empty strings are. I'll see what else I can add to figure out why this isn't writing a result.

noahgibbs commented 4 months ago

Update to latest main to check CI.

noahgibbs commented 4 months ago

This is now passing. I'll go ahead and merge.