Migrate pynessie testing to testcontainers by snazy · Pull Request #8 · projectnessie/pynessie

snazy · 2023-07-05T21:37:24Z

Also adopt tests to (not so) recent changes:

require common ancestor for merges
remove expected content code (was required for global state)

Also adopt tests to (not so) recent changes: * require common ancestor for merges * remove expected content code (was required for global state)

XN137

nice work!

some minor questions/suggestions

the assert len(logs) == 2 change seems the most problematic

XN137 · 2023-07-07T07:09:13Z

@@ -0,0 +1,3 @@
+[pytest]
+markers =
+    nessie: Test run against a Nessie container


nit: localserver, nessieserver or the like? just nessie seems confusing since every test is about (py)nessie

XN137 · 2023-07-07T07:19:45Z

+    assert_that(logs[0]["message"]).starts_with("Merged dev at ")
    logs = simplejson.loads(execute_cli_command(["--json", "log", "dev"]))
-    assert len(logs) == 1
+    assert_that(logs).is_length(2)


nit: pytest has smart asserts that provide full context on failure:

https://docs.pytest.org/en/6.2.x/assert.html

so personally dont see necessity for assert_that usage tbh

XN137 · 2023-07-07T07:21:20Z

-    main_hash = ref_hash("main")
-
-    merge_output = execute_cli_command(["merge", "dev", "-c", main_hash])
-    assert_that(merge_output).is_equal_to("Merge succeeded but legacy server did not respond with additional details.\n")


if we remove the test, should we also remove the code around it?
if we keep the code, would it be worth to add a mechanism to run this specific test against an older docker image?

XN137 · 2023-07-07T07:22:25Z

-    assert len(merge_response.source_commits) == 1
-    assert len(merge_response.details) == 1
-    assert_that(merge_response.details[0].source_commits[0]).is_equal_to(dev_hash)
+    assert_that(merge_response.details).is_length(1)


just wondering:
why can we longer assert dev_hash here?

source_commits + target_commits are no longer populated (removed in v2)

XN137 · 2023-07-07T07:26:20Z

    logs = simplejson.loads(execute_cli_command(["--json", "log", branch]))

-    assert len(logs) == 1
+    assert len(logs) == 2


doesnt this change now violate the comment 3 lines above i.e. "no commit to happen"?

Yea - the comment's wrong. Removed

XN137 · 2023-07-07T07:32:44Z


-@pytest.mark.vcr(record_mode="none")
+@pytest.mark.nessie
+@pytest.mark.skip("Test is no longer valid?")


if we can no longer inject error responses like this, then we might want to remove the tests instead of skipping

XN137 · 2023-07-07T07:41:10Z

+def pytest_runtest_setup(item: pytest.Item) -> None:
+    """Starts a Nessie container, if needed."""
+    if nessie_test_config.nessie_container is None and item.get_closest_marker("nessie") is not None:
+        nessie_test_config.nessie_container = NessieContainer(image=NessieContainer.DEFAULT_TEST_IMAGE)


is DEFAULT_TEST_IMAGE the latest "snapshot release" of the nessie image?

because DEFAULT_IMAGE is the latest official release (which we dont use in these tests now it seems).

i am wondering whether this may cause issues in the following cases:

snapshot is broken for some reason (suddenly pynessie PRs cant pass CI)

snapshot has an intentional breaking change (suddenly pynessie PRs cant pass CI even though they might be unrelated)

so just thinking out loud: maybe PR CI should run against a known good image version and on main branch we have scheduled CI run against unstable

but we can also figure out the issues once they actually happen

thinking about this again:

it may also be a question about compatibility guarantees i.e. what pynessie versions are working with what nessie server versions?
if we have some backwards compatibility guarantees we might want to run against those versions in CI.

previously both were released at the same time (but potentially broken due to VCR stuff not getting updated) but now with a separate repos (and versions?) it may be even less clear

The reason was actually the 2nd point: identify breaking changes w/ Nessie snapshot releases.

I was quite surprised how much stuff was actually no longer working - were just/mostly tests, but still.

Yea, we'd need CI against multiple release incl "latest main". But that can be done later, WDYT?

yes later is fine

XN137 · 2023-07-07T07:42:20Z

+
+def pytest_runtest_teardown(item: pytest.Item, nextitem: Optional[pytest.Item]) -> None:
+    """Stops the Nessie container, if one was started."""
+    if item.get_closest_marker("nessie") is not None and nessie_test_config.nessie_container is not None:


nit: the is not None is often unnecessarily verbose and can be omited here for example

since we dont have 0 or the empty string here ever

XN137 · 2023-07-07T07:43:08Z

-
-Run the Quarkus server with `-Dnessie.server.send-stacktrace-to-client=false` (this is the default) to reduce
-the size of the vcr cassettes and create less noise in updates related to REST api changes.
+PyNessie uses released Nessie Docker containers to execute tests.


nit: maybe mention NessieContainer class

XN137

latest commits lgtm

but CI fails linting:

pynessie/commands/merge.py:18:1: F401 'json' imported but unused

feel free to merge if that is the only change required to pass CI

snazy marked this pull request as draft July 5, 2023 21:37

snazy force-pushed the testcontainers branch 6 times, most recently from 1b06813 to b65a7e1 Compare July 6, 2023 09:32

Migrate pynessie testing to testcontainers

da7bc5d

Also adopt tests to (not so) recent changes: * require common ancestor for merges * remove expected content code (was required for global state)

snazy force-pushed the testcontainers branch from b65a7e1 to da7bc5d Compare July 6, 2023 09:59

snazy requested a review from XN137 July 6, 2023 09:59

snazy marked this pull request as ready for review July 6, 2023 09:59

XN137 previously approved these changes Jul 7, 2023

View reviewed changes

snazy added 5 commits July 7, 2023 10:19

nessie -> nessieserver

e66d2ce

remove legacy merge code + add merge-response to transplant

d4da98f

remove comment

1f2bab5

remove error-tests

0e3b085

update readme

f251549

snazy dismissed XN137’s stale review via f251549 July 7, 2023 08:19

snazy requested a review from XN137 July 7, 2023 08:19

XN137 previously approved these changes Jul 7, 2023

View reviewed changes

fix lint

5d31442

snazy dismissed XN137’s stale review via 5d31442 July 7, 2023 08:48

snazy merged commit 2fce9f3 into projectnessie:main Jul 7, 2023

snazy deleted the testcontainers branch July 7, 2023 08:55

snazy mentioned this pull request Jul 18, 2023

Contribute the Nessie-testcontainer to the testcontainers project projectnessie/nessie#7257

Open

Conversation

snazy commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XN137 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XN137 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

snazy commented Jul 5, 2023 •

edited

Loading