apache / apache/buildstream

Remove the workarounds for Click in testutils/runcli.py

Open
#729 1 comment 0 reactions 0 assignees View on GitHub
refactoring tests
Dominant language
Python
Stars
140
Forks
45
Avg merge
1d 3h
Merged PRs (30d)
6

Description

[See original issue on GitLab](https://gitlab.com/BuildStream/buildstream/-/issues/729)
In GitLab by [[Gitlab user @gokcennurlu]](https://gitlab.com/gokcennurlu) on Oct 25, 2018, 15:39

## Background

While discussing [__How to catch regressions in BuildStream's interactive interface__ on the mailing list](https://mail.gnome.org/archives/buildstream-list/2018-October/msg00053.html), I had a look at Click's changes between it's 6.7 and 7.0 (released on 25th of September), and BuildStream's [tests/testutils/runcli.py](https://gitlab.com/BuildStream/buildstream/blob/master/tests/testutils/runcli.py).

When we were using Click 6.7, [this commit](https://gitlab.com/BuildStream/buildstream/commit/7397c7721c5815aaaf6f82c474f85e3e8359bb8e) added a feature (separation of stderr/stdout) to testutils that didn't exist in Click yet by using pytest's internals and mimicking CliRunner interface.

[Over time](https://gitlab.com/BuildStream/buildstream/commits/master/tests/testutils/runcli.py), more BuildStream related helper functions were added.

On September 25th, [solution to the initial issue](https://github.com/pallets/click/commit/fdceb39d344603fc73a4d9761766b5701f69236d#diff-637edab3f8e0763a9874d17879193dc9), the separation of stderr/stdout, was released as part of 7.0 and BuildStream started to use it since there is no version pinning for that at the moment.

I believe it is a good time to remove the workarounds that we don't need them. By removing/simplifying `tests.testutils.runcli.Result` and simplifying the functions in `tests.testutils.runcli.Cli` we can remove lots of maintenance burden.

Finally, I haven't spent too much time on it but, this refactor can also help with the original issue (testing interactive parts of BuildStream) in the mailing list.

## Task description

[//]: # (Short summary of the action to be executed)

* [ ] Simplify or remove `tests.testutils.runcli.Result`
* [ ] Remove the need for `from _pytest.capture import MultiCapture, FDCapture`
* [ ] Clean up `tests.testutils.runcli.Cli.invoke()`, consider extending `click.testing.CliRunner` for `tests.testutils.runcli.Cli`

## Acceptance Criteria

[//]: # (Acceptance criteria should follow the S.M.A.R.T. principle https://en.wikipedia.org/wiki/SMART_criteria )

`tests.testutils.runcli.py` is free from workarounds are not necessary anymore.

----

Contributor guide

Open the contributing guide

Research direction

Start in tests/testutils/runcli.py, focusing on tests.testutils.runcli.Result, Cli, and Cli.invoke(), and compare their behavior with Click 7's click.testing.CliRunner. Check where MultiCapture and FDCapture are imported and used. Done means the file no longer relies on unnecessary Click workarounds while preserving the existing test utilities' behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
testing-qa
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.