exercism / nim-test-runner

GNU Affero General Public License v3.0
2 stars 3 forks source link

Feature(runner): capture output #48

Closed ynfle closed 3 years ago

ynfle commented 3 years ago

Supersedes #26 (my mess up because I deleted my fork before this was merged) Closes #13

iHiD commented 3 years ago

Thank you for this! 💙

@ee7 is knee-deep in Conflgiet atm (which I imagine is going to take their time up till launch) so I suggest that if you're broadly happy with this, we just get it merged down and then any improvements can be made later, rather than get too caught up in review cycles at this stage. I've therefore tagged @ErikSchierboom who can maybe give it a quick sanity check for you for now! :)

ynfle commented 3 years ago

@ee7 Had a requested a bunch of stuff that was requested and PRed on my branch (which I, unthoughtfully, deleted without waiting until this PR was merged) which can always be addressed later. @ee7, what do you think?

ee7 commented 3 years ago

we just get it merged down and then any improvements can be made later, rather than get too caught up in review cycles at this stage

The main part of the review is done. I just need to do a couple of things to fix this PR. Don't merge this yet.

I think I'll be able to merge this within the next 24 hours.

ynfle commented 3 years ago

we just get it merged down and then any improvements can be made later, rather than get too caught up in review cycles at this stage

The main part of the review is done. I just need to do a couple of things to fix this PR. Don't merge this yet.

I think I'll be able to merge this within the next 24 hours.

I rebased on main. What were you planning on fixing?

ee7 commented 3 years ago

. What were you planning on fixing?

First I'll make CI green again - it's currently failing on this PR.

See https://github.com/exercism/nim-test-runner/pull/49.

Then I'll make it so the new test cases actually run. Currently they're silently skipped - scroll down here and notice that, although the check is green, no output capture tests actually ran.

ee7 commented 3 years ago

Ping @iHiD - can you review this one too?

I'm setting this back to draft because I see:

Merging can be performed automatically with 1 approving review.

But I don't want automatic merging here - I want to merge this myself. And GitHub's automatic merging is new enough that I don't understand its subtleties.