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

Open the current merge request with lab mr b #221

Merged
merged 9 commits into from
Oct 18, 2018

Conversation

MartinDelille
Copy link
Contributor

This is a work in progress.

The current code fails with the following error:

../../go/src/github.com/zaquestion/lab/cmd/mr_browse.go:46:17: unknown field 'SourceBranch' in struct literal of type "github.com/zaquestion/lab/vendor/github.com/xanzy/go-gitlab".ListProjectMergeRequestsOptions

There is indeed a ListProjectMergeRequestsOptions.SourceBranch here: https://github.com/xanzy/go-gitlab/blob/master/merge_requests.go#L262

Unfortunately, I have a different file in vi vendor/github.com/xanzy/go-gitlab/merge_requests.go...

@MartinDelille MartinDelille changed the title [WIP] Open the current merge request with lab mr b Open the current merge request with lab mr b Sep 23, 2018
@MartinDelille
Copy link
Contributor Author

Ok I fixed the problems I had but now travis failed. I guess I should update the tests.

@zaquestion How can I run the test locally?

@MartinDelille
Copy link
Contributor Author

Ok it fails locally:

$ make test
bash -c "trap 'trap - SIGINT SIGTERM ERR; mv testdata/.git testdata/test.git; rm coverage-* 2>&1 > /dev/null; exit 1' SIGINT SIGTERM ERR; /Applications/Xcode.app/Contents/Developer/usr/bin/make internal-test"
rm coverage-* 2>&1 > /dev/null || true
rm: coverage-*: No such file or directory
mv testdata/test.git testdata/.git
go test -coverprofile=coverage-main.out -covermode=count -coverpkg ./... -run= github.com/zaquestion/lab/cmd github.com/zaquestion/lab/internal/...
The authenticity of host 'gitlab.com (35.231.145.151)' can't be established.
ECDSA key fingerprint is SHA256:HbW3g8zUjNSksFbqTiUWPWg2Bq1x8xdGUrliXFzSnUw.
Are you sure you want to continue connecting (yes/no)? The authenticity of host 'gitlab.com (35.231.145.151)' can't be established.
ECDSA key fingerprint is SHA256:HbW3g8zUjNSksFbqTiUWPWg2Bq1x8xdGUrliXFzSnUw.
Are you sure you want to continue connecting (yes/no)? The authenticity of host 'gitlab.com (35.231.145.151)' can't be established.
ECDSA key fingerprint is SHA256:HbW3g8zUjNSksFbqTiUWPWg2Bq1x8xdGUrliXFzSnUw.
Are you sure you want to continue connecting (yes/no)? The authenticity of host 'gitlab.com (35.231.145.151)' can't be established.
ECDSA key fingerprint is SHA256:HbW3g8zUjNSksFbqTiUWPWg2Bq1x8xdGUrliXFzSnUw.
Are you sure you want to continue connecting (yes/no)? The authenticity of host 'gitlab.com (35.231.145.151)' can't be established.
ECDSA key fingerprint is SHA256:HbW3g8zUjNSksFbqTiUWPWg2Bq1x8xdGUrliXFzSnUw.
Are you sure you want to continue connecting (yes/no)? The authenticity of host 'gitlab.com (35.231.145.151)' can't be established.
ECDSA key fingerprint is SHA256:HbW3g8zUjNSksFbqTiUWPWg2Bq1x8xdGUrliXFzSnUw.
Are you sure you want to continue connecting (yes/no)? ^C--- FAIL: Test_mrCmd (1.59s)
	root_test.go:72: ../testdata-2243430853747698137
    --- FAIL: Test_mrCmd/create (1.55s)
    	mr_test.go:34: POST https://gitlab.com/api/v4/projects/5694926/merge_requests: 409 {message: [Cannot Create: This merge request already exists: ["mr title"]]}
    		/diffs
    		PASS
    		coverage: 13.8% of statements in ./...

        Error Trace:    mr_test.go:35
    	Error:      	"POST https://gitlab.com/api/v4/projects/5694926/merge_requests: 409 {message: [Cannot Create: This merge request already exists: ["mr title"]]}
    	            	/diffs
    	            	PASS
    	            	coverage: 13.8% of statements in ./...
    	            	" does not contain "https://gitlab.com/lab-testing/test/merge_requests"
    	Test:       	Test_mrCmd/create
--- FAIL: Test_projectCreateCmd (1.01s)
	root_test.go:72: ../testdata-4018908385734095056
    --- FAIL: Test_projectCreateCmd/create (0.82s)
    	project_create_test.go:31: 2018/09/23 06:08:01 project_create.go:48: POST https://gitlab.com/api/v4/projects: 400 {message: {base: [PG::NotNullViolation: ERROR:  null value in column "approvals_before_merge" violates not-null constraint
    		DETAIL:  Failing row contains (8514392, testdata-4018908385734095056, testdata-4018908385734095056, , 2018-09-23 04:08:01.041227+00, 2018-09-23 04:08:01.041227+00, 1636476, 2002378, null, null, 10, f, null, null, 0, f, null, null, null, null, t, f, null, f, null, null, null, null, null, t, xji2xdxx7PukEsRxYrPo, null, t, 3600, f, t, f, null, null, t, f, null, nfs-file22, f, null, null, null, , f, null, t, t, 1, null, 11, null, null, null, null, null, f, null, t, null, null, null, null, null, t, t, null).
    		: INSERT INTO "projects" ("name", "path", "description", "visibility_level", "approvals_before_merge", "namespace_id", "creator_id", "runners_token", "created_at", "updated_at", "description_html", "cached_markdown_version", "packages_enabled", "archived", "resolve_outdated_diff_discussions", "container_registry_enabled", "repository_storage", "shared_runners_enabled", "only_allow_merge_if_all_discussions_are_resolved", "only_mirror_protected_branches") VALUES ('testdata-4018908385734095056', 'testdata-4018908385734095056', '', 10, NULL, 2002378, 1636476, 'xji2xdxx7PukEsRxYrPo', '2018-09-23 04:08:01.041227', '2018-09-23 04:08:01.041227', '', 11, 't', 'f', 'f', 't', 'nfs-file22', 't', 'f', 't') RETURNING "id"]}, {limit_reached: []}}

    	project_create_test.go:32: exit status 1
	project_create_test.go:50: failed to find project for cleanup: gitlab project not found
FAIL	github.com/zaquestion/lab/cmd	32.091s
?   	github.com/zaquestion/lab/internal/browser	[no test files]
ok  	github.com/zaquestion/lab/internal/config	0.025s	coverage: 6.6% of statements in ./...
make[1]: *** [internal-test] Error 1
make: *** [test] Error 1

@zaquestion
Copy link
Owner

Yeah the tests can be a bit annoying because they test against the real gitlab.com

You should just need to have an ssh key connected to a user on gitlab. Sorry there aren't better instructions on running the tests. You can select tests with

make test run=Test create

@MartinDelille
Copy link
Contributor Author

You should just need to have an ssh key connected to a user on gitlab.

I had one but I forgot that I changed my laptop since!

The problem now is that I don't have the right to create a merge request on this project.

The problem on travis is that there is an open merge request on the project with the same title: https://travis-ci.org/zaquestion/lab/builds/432038074#L576

I will try to close it via travis.

@MartinDelille
Copy link
Contributor Author

@zaquestion Maybe it'll be easier if you close it by hand!

@MartinDelille
Copy link
Contributor Author

@zaquestion Any idea what's going wrong? https://travis-ci.org/zaquestion/lab/builds/432954946#L572

@zaquestion
Copy link
Owner

Thats a pretty weird error -- I suspect it might be reproducible on master as well and have nothing to do with your code

@zaquestion
Copy link
Owner

Do you think you can add a test using https://github.com/zaquestion/lab/blob/master/cmd/mr_browse_test.go as guidance

@zaquestion
Copy link
Owner

@MartinDelille The test issue should be fixed by #222 -- can you rebase/update your branch. Sorry for the merge conflict!

@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #221 into master will decrease coverage by 0.5%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   68.09%   67.58%   -0.51%     
==========================================
  Files          41       41              
  Lines        2131     2147      +16     
==========================================
  Hits         1451     1451              
- Misses        533      548      +15     
- Partials      147      148       +1
Impacted Files Coverage Δ
cmd/mr_browse.go 48.38% <0%> (-51.62%) ⬇️

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 82c6531...8e8265f. Read the comment docs.

@MartinDelille
Copy link
Contributor Author

It fixed the problem!
I will try to add a test now that the build pass because I don't want to drop the code coverage! 😉

@MartinDelille
Copy link
Contributor Author

Do you have a test scenario to propose? I'm a kind of lost how to do that.

@zaquestion
Copy link
Owner

zaquestion commented Oct 9, 2018

@MartinDelille Sorry I just started a new job and haven't had much time to help out here. I'd suggest something like the existing test in https://github.com/zaquestion/lab/blob/master/cmd/mr_browse_test.go but don't pass an MR. You may have to checkout the mr test branch as well ( https://github.com/zaquestion/lab/blob/master/cmd/mr_test.go#L17-L23 )

func Test_mrBrowse(t *testing.T) {
	oldBrowse := browse
	defer func() { browse = oldBrowse }()

	browse = func(url string) error {
		require.Equal(t, "https://gitlab.com/zaquestion/test/merge_requests/<not sure what this would be>", url)
		return nil
	}

	mrBrowseCmd.Run(nil, []string{})
}

If you can create something like the above, I can pick it up and get it in the next release which needs to happen soon because of an annoying bug.

@codecov-io
Copy link

codecov-io commented Oct 14, 2018

Codecov Report

Merging #221 into master will decrease coverage by 0.5%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   68.09%   67.58%   -0.51%     
==========================================
  Files          41       41              
  Lines        2131     2147      +16     
==========================================
  Hits         1451     1451              
- Misses        533      548      +15     
- Partials      147      148       +1
Impacted Files Coverage Δ
cmd/mr_browse.go 48.38% <0%> (-51.62%) ⬇️

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 82c6531...9c3caf4. Read the comment docs.

@MartinDelille
Copy link
Contributor Author

Hi @zaquestion !

It works on my side locally. I tried again to write test but it requires times that I don't have. Maybe you could merge it without test so that people can at least use the feature?

We'll create an issue to create a test for this feature then (I don't like untested code too but sometimes we need to get thing done).

@zaquestion zaquestion merged commit 7430d77 into zaquestion:master Oct 18, 2018
@zaquestion zaquestion added this to the 0.14.0 milestone Oct 25, 2018
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.

3 participants