Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stricter error handling in tests #404

Merged
merged 10 commits into from Mar 8, 2022
Merged

Stricter error handling in tests #404

merged 10 commits into from Mar 8, 2022

Conversation

mislav
Copy link
Collaborator

@mislav mislav commented Jan 26, 2022

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 added 4 commits Jan 26, 2022
Tests that used to invoke methods on a `*expect.Console` instance now
invoke same-named methods that have error handling built in so that the
associated test is properly aborted if any of the methods happen to
fail. This kind of error handling was easier to add than adding explicit
error handling to every `c.Expect*()` and `c.Send*()` call site.
* Upgrade go-expect and associated libraries

* Explicitly test with supported Go versions

* Install a Go problem matcher

* Have skipped tests show up as warnings in Actions annotations

* Disable some flakey tests
@mislav
Copy link
Collaborator Author

mislav commented Jan 27, 2022

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 #327

@suarezjulian
Copy link

suarezjulian commented Feb 1, 2022

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 added 2 commits Feb 16, 2022
The test fails with:

    select_test.go:209: SendLine("") = write /dev/ptmx: input/output error

I cannot reproduce this failure on macOS locally.
https://github.com/AlecAivazis/survey/runs/5222853435
@mislav mislav requested a review from AlecAivazis Feb 16, 2022
@mislav
Copy link
Collaborator Author

mislav commented Feb 16, 2022

@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:

    image

@AlecAivazis
Copy link
Owner

AlecAivazis commented Mar 7, 2022

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

@mislav
Copy link
Collaborator Author

mislav commented Mar 8, 2022

@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
Copy link
Owner

AlecAivazis commented Mar 8, 2022

yea no problem at all

@AlecAivazis AlecAivazis closed this Mar 8, 2022
@AlecAivazis AlecAivazis reopened this Mar 8, 2022
@AlecAivazis AlecAivazis merged commit bcabe24 into master Mar 8, 2022
18 checks passed
@AlecAivazis AlecAivazis deleted the lint-pass branch Mar 8, 2022
@AlecAivazis
Copy link
Owner

AlecAivazis commented Mar 8, 2022

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
Copy link
Collaborator Author

mislav commented Mar 9, 2022

@mislav
Copy link
Collaborator Author

mislav commented Mar 9, 2022

@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
Copy link
Collaborator Author

mislav commented Mar 16, 2022

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

@AlecAivazis
Copy link
Owner

AlecAivazis commented Mar 16, 2022

Ah damn, sorry about that - fixing it now

@AlecAivazis
Copy link
Owner

AlecAivazis commented Mar 16, 2022

@mislav you should be good now

@mislav
Copy link
Collaborator Author

mislav commented Mar 17, 2022

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

The review requirement is still blocking my own PRs, though. Example: #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
Copy link
Owner

AlecAivazis commented Mar 17, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants