Skip to content

Add more repo search tests and /api/repo/search integration tests #2453

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

Closed

Conversation

Morlinest
Copy link
Member

Add some more tests. Also fixes #2446.

Two integration tests will not pass with current implementation of repo search api because it doesn't return all repositories accessible to user if he is not logged or is different from logged user

@Morlinest Morlinest force-pushed the feature/repo-search-tests branch 2 times, most recently from 0815508 to 44e6ba9 Compare September 2, 2017 22:27
@lunny lunny added this to the 1.3.0 milestone Sep 3, 2017
@lunny
Copy link
Member

lunny commented Sep 3, 2017

CI failed.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 3, 2017
@Morlinest
Copy link
Member Author

@lunny I told you it will and why.

I've added 8 new repositories to tests, 4 (2x public, 2x private) are owned by user15, 2 (1x public, 1x private) owned by user16 and user15 is collaborator in both of them and 2 repositories (1x public, 1x private) owned by organization which is owned by user15. That means if you search for public repositories accessible (and related) to user15, you should get 4 public repositories (2 owned by user15, 1 owned by user16 and 1 owned by organization he owns).

You should get only public (directly owned or accessible + related by any other non-private rules) repositories of user if it is anonymous search or logged user is different to that user. Am I thinking right?


assert.NotNil(t, repos)
assert.NoError(t, err)
assert.Equal(t, int64(3), count)
Copy link
Member Author

@Morlinest Morlinest Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny @lafriks I think, this should return 2, not 3 (only directly owned repositories). Organization repositories should be included with collaborate flag. What do you think? Same applies to line 112. Or add something like organization flag.

Copy link
Member

@lunny lunny Sep 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orgnization repositories should be in collaborate, I think it's right.

@Morlinest
Copy link
Member Author

Fixed tests and added 2 new tests for pages and pagesize limit

@Morlinest Morlinest force-pushed the feature/repo-search-tests branch from de6ae2d to 798be2d Compare September 4, 2017 10:02
@Morlinest
Copy link
Member Author

Tests failed as expected with current implementation of api and repo search function. I've done tests with #2371 and it will pass these new tests (#2371 rebased on this PR can be found in test-repo-search-implementation).

@Morlinest Morlinest force-pushed the feature/repo-search-tests branch 2 times, most recently from 44298dc to bb42912 Compare September 6, 2017 09:18
@Morlinest Morlinest force-pushed the feature/repo-search-tests branch from bb42912 to b9d56db Compare September 6, 2017 10:16
@Morlinest
Copy link
Member Author

Merged to #2371.

@Morlinest Morlinest closed this Sep 7, 2017
@lunny lunny removed this from the 1.3.0 milestone Sep 8, 2017
@Morlinest Morlinest deleted the feature/repo-search-tests branch September 20, 2017 15:07
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration test TestAPISearchRepoNotLogin doesn't work
3 participants