test_landings: increase git landing coverage (bug 1934485) #190

shtrom · 2025-01-06T06:08:52Z

upgrade landings tests to work with both Hg and Git repos
fix a few simple bugs in the Git implementation

… index

src/lando/api/tests/conftest.py

src/lando/api/tests/test_landings.py

cgsheeh · 2025-01-10T17:33:48Z

src/lando/api/tests/test_landings.py

+def _scm_get_previous_hash(scm: AbstractSCM) -> str:
+    if scm.scm_type() == SCM_TYPE_HG:
+        return HgSCM.run_hg(scm, ["log", "-r", "tip^", "-T", "{node}"]).decode("utf-8")
+    return GitSCM._git_run("rev-parse", "HEAD^", cwd=scm.path)


We should have an AbstractSCM.get_previous_hash instead of this function which checks for the specific implementation.

If these are only used in tests, I would hesitate to put them directly in the main implementation, but maybe they can be added in a mixin or a test class that extends AbstractSCM. Again, depends if they are only used in tests.

I had the same thought as @zzzeid here. Same for the other one that gets the whole commit message.

Both are only used in tests at the moment.

I don't think it's an issue that it's a function only used in tests, worst case we could add a comment pointing that out in the docstring. This way it seems like we are going against the purpose of the AbstractSCM. I'm not too fussed about it either way though, happy to go with whatever you decide.

cgsheeh · 2025-01-10T17:35:20Z

src/lando/api/tests/test_landings.py

+def _scm_get_last_commit_message(scm: AbstractSCM) -> str:
+    if scm.scm_type() == SCM_TYPE_HG:
+        return HgSCM.run_hg(scm, ["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8")
+    return GitSCM._git_run("log", "--pretty=%B", "HEAD^..", cwd=scm.path)


Same as above, these methods should go on the AbstractSCM and the implementations.

Same comment here as in _scm_get_previous_hash -- i.e., maybe those should be added to a mixin or test class.

src/lando/main/tests/conftest.py

src/lando/api/tests/test_landings.py

zzzeid · 2025-01-10T20:20:30Z

src/lando/api/tests/test_landings.py

+def _scm_get_previous_hash(scm: AbstractSCM) -> str:
+    if scm.scm_type() == SCM_TYPE_HG:
+        return HgSCM.run_hg(scm, ["log", "-r", "tip^", "-T", "{node}"]).decode("utf-8")
+    return GitSCM._git_run("rev-parse", "HEAD^", cwd=scm.path)


If these are only used in tests, I would hesitate to put them directly in the main implementation, but maybe they can be added in a mixin or a test class that extends AbstractSCM. Again, depends if they are only used in tests.

src/lando/api/tests/test_landings.py

zzzeid · 2025-01-10T20:27:23Z

src/lando/main/scm/git.py

+            if "nothing to commit, working tree clean" in exc.out:
+                return []


Is there a more rigorous way of doing this check? Is there a specific error code we could be checking when raising SCMException? Looking at your implementation of _git_run, looks like we are raising this exception specifically based on a return code, and perhaps we shouldn't be raising this as an exception at all from the get go.

The initial implementation had a conditional to allow throwing an exception or not, but I realised at the time that we always were throwing an exception, so I removed the parameter.

We could re-add it, but I can't find documentation about git commit's error codes (at the moment it return 1 for this case), so we'd still have to inspect the error message to determine the cause of the error (FWIW, this is also how we do it for Mercurial) https://github.com/mozilla-conduit/lando/blob/main/src/lando/main/scm/hg.py#L372-L377

src/lando/main/tests/conftest.py

src/lando/api/tests/conftest.py

Co-authored-by: Connor Sheehan <[email protected]>

Updated mc_repo factory, fixed nits.

shtrom · 2025-01-13T00:53:17Z

Hum. The tests in GitHub fail https://github.com/mozilla-conduit/lando/pull/190/checks#step:6:390. They were fine on last week, and still pass locally. Something's odd with GHActions, I think.

…st-coverage

shtrom · 2025-01-15T06:14:45Z

@zzzeid @cgsheeh test pass with the fix to test_hg. This should be good for one last look.

cgsheeh · 2025-01-16T17:12:53Z

src/lando/api/tests/test_landings.py

+def _scm_get_previous_hash(scm: AbstractSCM) -> str:
+    if scm.scm_type() == SCM_TYPE_HG:
+        return HgSCM.run_hg(scm, ["log", "-r", "tip^", "-T", "{node}"]).decode("utf-8")
+    return GitSCM._git_run("rev-parse", "HEAD^", cwd=scm.path)


I don't think it's an issue that it's a function only used in tests, worst case we could add a comment pointing that out in the docstring. This way it seems like we are going against the purpose of the AbstractSCM. I'm not too fussed about it either way though, happy to go with whatever you decide.

zzzeid

nit: in first line of commit message: test -> test_landings.

src/lando/main/tests/conftest.py

…3) (#147)

Addressed

shtrom added 3 commits January 10, 2025 14:55

conftest: dos2unix conversion

fb80301

test-landings: add hg_repo_mc fixture

b4bd0ed

test_landings: add repo_mc parametrable factory

e36a839

shtrom force-pushed the bug1934485/landing-test-coverage branch from aea01be to 8411436 Compare January 10, 2025 03:57

shtrom added 4 commits January 10, 2025 15:03

tests: add git_repo to landings tests

f125504

test_git: initialise test_repo with seed patch

484658b

tests: don't rstrip diff fixtures (git requires a trailing newline)

599f216

*SCM.apply_patch: add suffix to temporary files

a13da75

shtrom force-pushed the bug1934485/landing-test-coverage branch from 8411436 to 15c0d4e Compare January 10, 2025 04:03

shtrom added 4 commits January 10, 2025 15:07

test_landings: allow create_patch_revision to fetch normal patches by…

40b6d0c

… index

test: ignore denyCurrentBranch in git repo fixture

72f1cde

GitSCM: set Git user on local clone

8375fc2

GitSCM.update_repo: use git fetch rather than pull

2588f72

shtrom force-pushed the bug1934485/landing-test-coverage branch from 15c0d4e to 18e9068 Compare January 10, 2025 04:07

shtrom requested review from cgsheeh and zzzeid January 10, 2025 04:10

shtrom force-pushed the bug1934485/landing-test-coverage branch from 18e9068 to 2588f72 Compare January 10, 2025 04:46

shtrom mentioned this pull request Jan 10, 2025

AbstractSCM: move process_merge_conflict to SCM (bug 1940876) #191

Merged

cgsheeh previously requested changes Jan 10, 2025

View reviewed changes

zzzeid previously requested changes Jan 10, 2025

View reviewed changes

shtrom and others added 4 commits January 13, 2025 11:08

Update src/lando/api/tests/test_landings.py

a5cff9f

Co-authored-by: Connor Sheehan <[email protected]>

Update src/lando/main/tests/conftest.py

be12b48

Co-authored-by: Connor Sheehan <[email protected]>

fixup! landing_worker: add and use git scm support (bug 1895523) (#147)

9a7f1cd

test_landings: update repo_mc factory to allow parameters

4e8e277

shtrom requested a review from zzzeid January 13, 2025 00:49

shtrom requested a review from cgsheeh January 13, 2025 00:50

Merge remote-tracking branch 'origin/main' into bug1934485/landing-te…

11bc37a

…st-coverage

cgsheeh approved these changes Jan 16, 2025

View reviewed changes

zzzeid previously requested changes Jan 16, 2025

View reviewed changes

src/lando/main/tests/conftest.py Outdated Show resolved Hide resolved

shtrom added 2 commits January 17, 2025 12:18

fixup! tests: add git_repo to landings tests

e7428e2

fixup! fixup! landing_worker: add and use git scm support (bug 189552…

ab884d9

…3) (#147)

shtrom changed the title ~~test: increase git landing coverage (bug 1934485)~~ test_landings: increase git landing coverage (bug 1934485) Jan 17, 2025

shtrom merged commit d780a9e into main Jan 17, 2025
1 check passed

shtrom deleted the bug1934485/landing-test-coverage branch January 17, 2025 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_landings: increase git landing coverage (bug 1934485) #190

test_landings: increase git landing coverage (bug 1934485) #190

shtrom commented Jan 6, 2025 •

edited

Loading

cgsheeh Jan 10, 2025

zzzeid Jan 10, 2025

shtrom Jan 13, 2025

cgsheeh Jan 16, 2025

cgsheeh Jan 10, 2025

zzzeid Jan 10, 2025

zzzeid Jan 10, 2025

zzzeid Jan 10, 2025

shtrom Jan 13, 2025

shtrom commented Jan 13, 2025

shtrom commented Jan 15, 2025

cgsheeh Jan 16, 2025

zzzeid left a comment

		if "nothing to commit, working tree clean" in exc.out:
		return []

test_landings: increase git landing coverage (bug 1934485) #190

test_landings: increase git landing coverage (bug 1934485) #190

Conversation

shtrom commented Jan 6, 2025 • edited Loading

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

shtrom commented Jan 13, 2025

shtrom commented Jan 15, 2025

Choose a reason for hiding this comment

zzzeid left a comment

Choose a reason for hiding this comment

shtrom commented Jan 6, 2025 •

edited

Loading