Add test helpers, rewrite integration tests #100

Kyle-Verhoog · 2020-11-28T23:58:47Z

Add test helpers to do proper input/output testing.

codecov-io · 2020-11-28T23:59:58Z

Codecov Report

Merging #100 (0bd9ccd) into master (27fd17d) will decrease coverage by 5.92%.
The diff coverage is 96.93%.

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
- Coverage   95.37%   89.44%   -5.93%     
==========================================
  Files           7        9       +2     
  Lines         584      597      +13     
==========================================
- Hits          557      534      -23     
- Misses         27       63      +36

Impacted Files	Coverage Δ
tests/test_cli.py	`97.91% <ø> (-2.09%)`	⬇️
tests/proctest.py	`88.37% <88.37%> (ø)`
tests/test_integration.py	`100.00% <100.00%> (ø)`
riot/riot.py	`81.53% <0.00%> (-10.00%)`	⬇️
riot/cli.py	`90.00% <0.00%> (-5.00%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27fd17d...d04ac50. Read the comment docs.

riotfile.py

tests/proctest.py

tests/test_integration.py

brettlangdon · 2021-05-21T21:46:45Z

tests/test_integration.py

+    assert result.stderr == ""
+    assert result.returncode == 0
+
+    result = run("riot list", cwd=tmp_path)


we should get the same for riot run and riot generate right?

Correct, are you suggesting that we should also check those here? Would be an easy parametrization

tests/test_integration.py

brettlangdon · 2021-05-21T21:53:19Z

tests/test_integration.py

+test  .* 'pkg1==1.0'
+test  .* 'pkg1==2.0'


We should know this exact string no? since this is the python version and we know the currently running version?

The reasoning here is that the interpreter could be platform dependent. Right now it'd be fine because the output there would probably just be Interpreter(_hint="3") but in #137 we'd also have the path to the interpreter.

What we can do is at least match on Interpreter(.*) or similar. WDYT?

Makes me wonder if we need better output here than what we have now repr(Interpreter)

could make this more of a consistent format

not sure I am suggesting a specific change right now, just talking out loud.

Reduces the boilerplate of having to specify cwd every time when commands are 99% of the time run in the tmp_path.

Protocol was only introduced in Python 3.8. typing_extensions makes it available for older Pythons.

tests/test_integration.py

brettlangdon · 2021-05-22T13:26:06Z

tests/test_integration.py

+test  .* 'pkg1==1.0'
+test  .* 'pkg1==2.0'


Makes me wonder if we need better output here than what we have now repr(Interpreter)

could make this more of a consistent format

brettlangdon · 2021-05-22T13:26:20Z

tests/test_integration.py

+test  .* 'pkg1==1.0'
+test  .* 'pkg1==2.0'


not sure I am suggesting a specific change right now, just talking out loud.

brettlangdon · 2021-05-22T13:29:41Z

tests/test_integration.py

+    assert result.returncode == 1
+
+
+def test_bad_interpreter(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:


Suggested change

def test_bad_interpreter(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:

@with_riotfile(

"""

from riot import Venv

venv = Venv(

...

)

"""

, filename="bad-riotfile.py", # Obviously the default is `riotfile.py`

)

def test_bad_interpreter(riotfile: pathlib.Path, tmp_run: _T_TmpRun) -> None:

Suggested change

def test_bad_interpreter(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:

@with_riotfile_fixture("some-file-in-data-dir.py)

def test_bad_interpreter(riotfile: pathlib.Path, tmp_run: _T_TmpRun) -> None:

Just writing something down as a trial to see how it felt, not suggesting a change here.

Co-authored-by: Brett Langdon <me@brett.is>

jd

LGTM.
Adding manual merge in case we want another review.
Feel free to remove it if everyone's happy.

Kyle-Verhoog added the no-changelog This does not need a user visible changelog label Nov 28, 2020

Kyle-Verhoog requested a review from a team as a code owner November 28, 2020 23:58

Kyle-Verhoog force-pushed the tests branch from be9f1c6 to 2f17c0b Compare November 29, 2020 00:06

Add test helpers, rewrite integration tests

d04ac50

Kyle-Verhoog force-pushed the tests branch 2 times, most recently from 0bd9ccd to d04ac50 Compare November 29, 2020 00:32

jd suggested changes Nov 30, 2020

View reviewed changes

riotfile.py Outdated Show resolved Hide resolved

tests/proctest.py Outdated Show resolved Hide resolved

tests/proctest.py Outdated Show resolved Hide resolved

tests/proctest.py Outdated Show resolved Hide resolved

tests/proctest.py Outdated Show resolved Hide resolved

Kyle-Verhoog added 3 commits May 21, 2021 17:25

Remove custom proctest, use tmp_path pytest fixture

e624d14

Merge remote-tracking branch 'origin/master' into tests

217031e

Fix linting and tests

0ad947d

Kyle-Verhoog force-pushed the tests branch from a352908 to 0ad947d Compare May 21, 2021 21:53

brettlangdon reviewed May 21, 2021

View reviewed changes

Kyle-Verhoog added 4 commits May 21, 2021 18:23

set shell=True when args is a str

f322ec5

Add tmp_run fixture

7b2bb88

Reduces the boilerplate of having to specify cwd every time when commands are 99% of the time run in the tmp_path.

Add typing for tmp_run

e692372

Use typing_extensions for Protocol

f723e9c

Protocol was only introduced in Python 3.8. typing_extensions makes it available for older Pythons.

brettlangdon reviewed May 22, 2021

View reviewed changes

Remove unused fixtures

8e5de95

Co-authored-by: Brett Langdon <me@brett.is>

jd added the manual merge Do not merge automatically label May 25, 2021

jd approved these changes May 25, 2021

View reviewed changes

brettlangdon removed the manual merge Do not merge automatically label May 25, 2021

mergify bot merged commit 5ad6e6d into DataDog:master May 25, 2021

Kyle-Verhoog deleted the tests branch May 25, 2021 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test helpers, rewrite integration tests #100

Add test helpers, rewrite integration tests #100

Kyle-Verhoog commented Nov 28, 2020 •

edited

Loading

codecov-io commented Nov 28, 2020 •

edited

Loading

brettlangdon May 21, 2021

Kyle-Verhoog May 21, 2021

brettlangdon May 21, 2021

Kyle-Verhoog May 21, 2021

brettlangdon May 22, 2021

brettlangdon May 22, 2021

brettlangdon May 22, 2021

brettlangdon May 22, 2021

brettlangdon May 22, 2021

jd left a comment

		assert result.returncode == 1


		def test_bad_interpreter(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:

-def test_bad_interpreter(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:
+@with_riotfile(
+    """
+from riot import Venv
+venv = Venv(
+    ...
+)
+    """
+    , filename="bad-riotfile.py",  # Obviously the default is `riotfile.py`
+)
+def test_bad_interpreter(riotfile: pathlib.Path, tmp_run: _T_TmpRun) -> None:

	def test_bad_interpreter(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:
	@with_riotfile_fixture("some-file-in-data-dir.py)
	def test_bad_interpreter(riotfile: pathlib.Path, tmp_run: _T_TmpRun) -> None:

Add test helpers, rewrite integration tests #100

Add test helpers, rewrite integration tests #100

Conversation

Kyle-Verhoog commented Nov 28, 2020 • edited Loading

codecov-io commented Nov 28, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jd left a comment

Choose a reason for hiding this comment

Kyle-Verhoog commented Nov 28, 2020 •

edited

Loading

codecov-io commented Nov 28, 2020 •

edited

Loading