Stricter error handling in tests

[mislav]

Currently, the test suite is flakey, impeding the merge of PRs due to intermittent failures of required jobs. Example: https://github.com/AlecAivazis/survey/runs/4952430024?check_suite_focus=true

I'm trying to track down the cause of flakes, but it's not easy due to a lot of them being caused by test suite timeouts (and thus, no specific test in particular). So I made a pass of strengthening error handling, particularly when it comes to tests:

• I've used golangci-lint tool locally to find and address violations, mostly those were it pointed out that errors are not handled. In cases when we want to explicitly ignore the error, I use the _ = foo() assignment.
• Calls to expect.Console methods were never checked for error return values. I've made a light wrapper around expect.Console that transparently reports test failures for any error encountered. This is a step in ensuring that no test is broken and stuck on I/O until it times out.
• I've added t.Helper() calls to test helper functions so that they are excluded from being reported as a source of a test failure. We're more interested in where the failure originates in the test itself.

[mislav]

This PR has now increased considerably in size since I've merged a nested PR that upgrades go-expect and associated libraries and marked a few tests as flakey. The test suite still fails intermittently, but I think this is progress towards a better test suite.

I still cannot figure out some prompts are hanging and causing timeouts. So far, a lot of the tests intermittently failing (locally and on CI) are related to https://github.com/AlecAivazis/survey/pull/327

[suarezjulian]

Following this with a lot of interest. I don't have timeout issues as in this PR, but I have timing issues (Ubuntu 21.10 only as well) where I see duplicated output for confirm prompt :

On Ubuntu I sometimes see this two lines with a confirm prompt :

(stdout)
?Do you like pizza (y/N)? y
?Do you like pizza? Yes

On Mac OS it is always

(stdout)
?Do you like pizza? Yes

My tests are setup exactly as in https://github.com/AlecAivazis/survey/blob/master/survey_posix_test.go#L15

[mislav]

@AlecAivazis This is now ready, although:

1. The changeset is huge. Sorry! It mostly just brings the test suite dependencies up to date and tightens up error handling. There should be no perceivable change to users of Survey at runtime.
2. It explicitly disables 2 tests that are flakey and that I was unable to debug due to not being able to reproduce locally.

3. The updated build matrix changed the names of test jobs, so now the jobs required by the branch protection policy do not "pass" because they are under a different name. This will require you changing the repo settings to allow me to merge this PR:

   

[AlecAivazis]

Sorry for lagging on this - changes look good to me!

[mislav]

@AlecAivazis Thanks for the approval! I'm still unable to merge this PR due to required status checks (see my previous comment). Are you able to help me with this?

[AlecAivazis]

yea no problem at all

[AlecAivazis]

I tried to find the setting to make you an admin but couldn't locate it. do you know where that is by chance?

[mislav]

@AlecAivazis Unfortunately, there is no way to add additional admins to a personal repository.

[mislav]

@AlecAivazis You could perhaps remove the branch protection rules so it's easier for me to merge changes even if "required" changes are not passing. Sometimes there is a flaky test blocking the merge, even after all the work I've done in this PR :(( https://github.com/AlecAivazis/survey/runs/5479779404?check_suite_focus=true

Also, if it's fine with you, I'd like to be able to merge my own PRs even without your approval, in case you are away for longer periods of time! 🙇

[mislav]

Ping @AlecAivazis ☝️ there are still PRs that I'm unable to merge due to the above

[AlecAivazis]

Ah damn, sorry about that - fixing it now

[AlecAivazis]

@mislav you should be good now

[mislav]

@AlecAivazis Thanks; CI checks are not blocking me anymore.

The review requirement is still blocking my own PRs, though. Example: https://github.com/AlecAivazis/survey/pull/397/

If you do not intend to review each one of my PRs in a timely manner, which is absolutely fine, you could consider removing the review requirement. I promise I won't be making outrageous or incompatible changes to the library.

[AlecAivazis]

Whoops, i was hoping you would be able to be one of the people who could review it, but i guess not. I've lifted the requirement - let me know if there's anything else I need to do