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

Added test environment for mssql #4282

Merged
merged 43 commits into from Dec 12, 2018
Merged

Added test environment for mssql #4282

merged 43 commits into from Dec 12, 2018

Conversation

kolaente
Copy link
Member

This pr adds a test step in drone to run tests with mssql db.

Even if you dont like Microsoft (like me), IMHO we should at least have tests for mssql dbs or not support it at all.

.drone.yml Outdated
@@ -360,7 +360,7 @@ services:
image: microsoft/mssql-server-linux:latest
environment:
- ACCEPT_EULA=Y
- SA_PASSWORD=M$wantsaSecurePassword
- SA_PASSWORD=M$wantsaSecurePassword1
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to escape $ character otherwise it tries to connect with password MwantsaSecurePassword1

Copy link
Member

Choose a reason for hiding this comment

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

but maybe not it this file but in app.ini

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing that out, I thinks thats the issue.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 20, 2018
@lafriks
Copy link
Member

lafriks commented Jun 20, 2018

Database is not created so gitea is not able to connect:
error: mssql: Cannot open database "testgitea" that was requested by the login. Using the user default database "master" instead.

@kolaente
Copy link
Member Author

@lafriks oh ok. Drone shows a different error message though... But I'll try with your recommondation.

@kolaente
Copy link
Member Author

kolaente commented Jun 20, 2018

Now I get that weired error:

Failed to initialize ORM engine: sync database struct error: Unknown colType SYSNAME for MSreplication_options - optname

Is that an issue with xorm, the test I added, or Gitea?

\cc @lunny looks like a xorm issue.

@lafriks
Copy link
Member

lafriks commented Jun 20, 2018

@kolaente Gitea does not create database so docker service needs to do it.

@lunny
Copy link
Member

lunny commented Jun 21, 2018

@kolaente I will investigate it. And as @lafriks said, you have to create database manually.

@lunny lunny added this to the 1.6.0 milestone Jul 4, 2018
@lunny lunny mentioned this pull request Jul 4, 2018
7 tasks
@lunny
Copy link
Member

lunny commented Jul 4, 2018

@kolaente you have to add some SQL to create the database at first.

@@ -172,6 +172,20 @@ pipeline:
when:
event: [ push, tag, pull_request ]

test-mssql:
Copy link
Member

Choose a reason for hiding this comment

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

Edit: I was being lazy, pay attention to @JonasFranzDEV as his solution is better than mine.
before the test steps (look at group test) you could have a step that creates the DB like:

mssql-create-db:
   image: microsoft/mssql-tools
   commands:
    - sqlcmd -S mssql -U sa -P MwantsaSecurePassword1 -Q "CREATE DATABASE master"
   when:
      event: [ push, tag, pull_request ]

I haven't tested the above (or indentation) but something like it should work

Copy link
Member

Choose a reason for hiding this comment

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

The better option might be to create the database in code like we do for pgsql and mysql:
https://github.com/go-gitea/gitea/blob/master/integrations/integration_test.go#L110

Copy link
Member

Choose a reason for hiding this comment

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

I've implemented it and the test works now

Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Copy link
Member

@jonasfranz jonasfranz left a comment

Choose a reason for hiding this comment

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

The test environment works but the test fails for other reasons.

@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 5, 2018
@chdxD1
Copy link
Contributor

chdxD1 commented Dec 11, 2018

@lunny Thanks for the pull request (this is also an issue but not the underlying one) go-xorm/xorm#1173
Tested this locally and all tests seem to be OK.

@lunny
Copy link
Member

lunny commented Dec 11, 2018

@chdxD1 thanks, merged xorm's PR. But we still need a PR to update xorm, maybe this PR? @kolaente

@lunny lunny reopened this Dec 11, 2018
@kolaente
Copy link
Member Author

@lunny I'll update it

@kolaente
Copy link
Member Author

Now the build fails, does anyone know if xorm changed significant amounts of code so our code is invalid?

@chdxD1
Copy link
Contributor

chdxD1 commented Dec 11, 2018

@kolaente Could you please run a dep ensure locally and remove the string from the interface https://github.com/kolaente/gitea/blob/master/models/models.go#L39

@kolaente
Copy link
Member Author

@chdxD1 Still fails, but because apparantly models/repo_watch.go:34 still uses exec
@lunny Is there a reason this used e.Exec instead of x.Exec? x is available in that context.

@kolaente
Copy link
Member Author

Build should work now!

@chdxD1
Copy link
Contributor

chdxD1 commented Dec 11, 2018

@kolaente Despite repo_watch using e.Exec: This is the relevant change from xorm (for reviewers): go-xorm/xorm@4ce90f9
The first parameter is beeing checked for type string (so all calls to the Exec function will still work)

@kolaente
Copy link
Member Author

@chdxD1 makes sense, I just thought it had beed removed in favour of x.SQL.

@codecov-io
Copy link

codecov-io commented Dec 11, 2018

Codecov Report

Merging #4282 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4282      +/-   ##
==========================================
- Coverage   37.62%   37.59%   -0.04%     
==========================================
  Files         319      319              
  Lines       46949    46949              
==========================================
- Hits        17666    17651      -15     
- Misses      26769    26791      +22     
+ Partials     2514     2507       -7
Impacted Files Coverage Δ
models/models.go 54.54% <0%> (ø) ⬆️
modules/log/file.go 42.73% <0%> (-24.79%) ⬇️
routers/repo/view.go 46.01% <0%> (+1.22%) ⬆️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️
models/repo_indexer.go 47.88% <0%> (+3.38%) ⬆️

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 b1f3685...96f7f86. Read the comment docs.

@kolaente
Copy link
Member Author

CI passes! 🎉 🎉 🎉 🎉 🎉 🎉

@techknowlogick techknowlogick removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Dec 11, 2018
@lafriks
Copy link
Member

lafriks commented Dec 11, 2018

@lunny need your approval

@lunny lunny merged commit 6db7dbd into go-gitea:master Dec 12, 2018
@lunny
Copy link
Member

lunny commented Dec 12, 2018

Thanks all of you!!!

@kolaente kolaente deleted the mssql-test branch December 12, 2018 09:36
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 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

9 participants