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

improve integration test to resue models/fixtures and store git repos with tests #1627

Merged
merged 3 commits into from Apr 28, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Apr 27, 2017

Since there are two fixtures, I will remove the fixtures in the integration.tar.gz and only keep git repos in the .tar.gz. Also I keep the .tar.gz with the tests and not download from network so that we can keep the meta data with the tests synchronism.

@andreynering
Copy link
Contributor

Trusted LGTM

I agree that tests should not require internet connection.

Different fixture folders are not necessarally bad if you have different data there. Are fixtures of integration tests different from models @ethantkoenig ?

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 27, 2017
@andreynering
Copy link
Contributor

Also, maybe get rid of the .tar.gz file, and just store it as is like https://github.com/ethantkoenig/gitea-integration ?

Could be in another PR to not block this one.

@sapk
Copy link
Member

sapk commented Apr 27, 2017

Following @andreynering proposition of storing file and not .tar.gz we could vendor it to "easely" follow modification to data set. And like you propose doing it in an other PR is good also.

@sapk
Copy link
Member

sapk commented Apr 27, 2017

I haven't check content of gitea-integration.tar.gz but I can trust LGTM.

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 27, 2017
@lunny
Copy link
Member Author

lunny commented Apr 27, 2017

I think store as .tar.gz is better since there .git folders in the repos.

@appleboy
Copy link
Member

LGTM

@andreynering
Copy link
Contributor

I think store as .tar.gz is better since there .git folders in the repos.

@lunny I they were called .git, Git would reconize them as Git repos. But they're actually called {{repoName}}.git, so no problem.

Copy link
Member

@ethantkoenig ethantkoenig left a comment

Choose a reason for hiding this comment

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

This won't work, since the fixtures in models/fixtures don't match the repos in gitea-integration.tar.gz

If we want to reuse fixtures between unit tests and integration tests, we will need to update gitea-integration.tar.gz accordingly.

@lunny
Copy link
Member Author

lunny commented Apr 28, 2017

@ethantkoenig yes, you are right. I have updated the file integration.tar.gz on this PR.

@lunny
Copy link
Member Author

lunny commented Apr 28, 2017

@andreynering sure, you are right. There is no .git folder only xxxx.git folder. So seems it's no need to store as .tar.gz file.

@ethantkoenig
Copy link
Member

@lunny We're still missing repositories for every repo other than user2/repo1

Is there a reason we need to use the same fixtures for unit tests and integration tests?

@lunny
Copy link
Member Author

lunny commented Apr 28, 2017

@ethantkoenig use the same fixtures will let us test the data entirely. Two fixtures are always confusing me which one I'm testing. The missing repository could be added in other future PRs. Currently It's enough.

@strk
Copy link
Member

strk commented Apr 28, 2017

Could those git repositories be created at runtime rather than being included in the source ?
Just a setup phase issuing git commands...

@lunny
Copy link
Member Author

lunny commented Apr 28, 2017

@strk, I store it in a .tag.gz before and unzip it when test is running. Just changed to this form.

@sapk
Copy link
Member

sapk commented Apr 28, 2017

It would be better to have fixture data in a other repo maybe but this good enough to start using integration test.

@ethantkoenig
Copy link
Member

@sapk That was the solution I had in mind with https://github.com/ethantkoenig/gitea-integration. I had hoped/planned to eventually transfer ownership of that repo to @go-gitea

Copy link
Member

@ethantkoenig ethantkoenig left a comment

Choose a reason for hiding this comment

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

Not a huge fan of having inconsistencies between fixtures and repositories, but I'll approve to help move things along.

@bkcsoft
Copy link
Member

bkcsoft commented Apr 28, 2017

I'd really prefer if we generated repositories based on the repos. But that might not be doable?

@lunny
Copy link
Member Author

lunny commented Apr 28, 2017

@ethantkoenig I would like keep the testdata here for version control. Put it a seperate repo seems not necessary. I will merge this at first. Please feel free to discuss here.

@lunny lunny merged commit fca7ddc into go-gitea:master Apr 28, 2017
@lunny
Copy link
Member Author

lunny commented Apr 28, 2017

@bkcsoft Do you mean generate repository according the fixtures records?

@bkcsoft
Copy link
Member

bkcsoft commented Apr 28, 2017

@lunny yes. Basically read the fixtures into arrays and create repos out of that

@lunny
Copy link
Member Author

lunny commented Apr 28, 2017

I'm thinking generate the visit URL according fixtures repos so that we can test all the routes.

@lunny lunny deleted the lunny/improve_integration_meta_data branch April 28, 2017 13:31
@ethantkoenig
Copy link
Member

ethantkoenig commented Apr 28, 2017

I'm not sure how generating repositories from fixtures would work. It seems that there is a lot of state stored in repositories (mostly commit history) that can't be inferred from fixtures.

For example, for each merged entry in the pull_request table, we would need to somehow generate a commit whose SHA-1 hash matches the corresponding merged_commit_id.

@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/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants