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

Stop various tests from adding to the source tree #9515

Merged
merged 2 commits into from
Dec 28, 2019

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Dec 27, 2019

Instead of just adding test generated files to .gitignore. We should prevent them from being produced in the first place.

No test should ever write to the source code tree!

Fixes #9517

Instead of just adding test generated files to .gitignore prevent
them from being produced in the first place.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 27, 2019
@zeripath zeripath mentioned this pull request Dec 27, 2019
19 tasks
@zeripath
Copy link
Contributor Author

I wish there was an easy way to ensure that this does not happen. Perhaps we should be running the tests after chmoding the source tree to prevent this sort of behaviour?!

@GiteaBot GiteaBot 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 Dec 27, 2019
@sapk
Copy link
Member

sapk commented Dec 27, 2019

@zeripath We could do a similar check in CI than the css/js does (or did) for unchanged/added file to the git repo after the tests ran.

@zeripath
Copy link
Contributor Author

Yeah I guess that's a reasonable thing to do but it doesn't prevent tests from creating files in the tree and then deleting them or just adding them to gitignore.

@zeripath
Copy link
Contributor Author

@sapk something like this:

diff --git a/.drone.yml b/.drone.yml
index 7a844af8e..ae6a5f9b3 100644
--- a/.drone.yml
+++ b/.drone.yml
@@ -105,6 +105,7 @@ steps:
     image: golang:1.13
     commands:
       - make unit-test-coverage
+      - make test-check
     environment:
       GOPROXY: off
       TAGS: bindata sqlite sqlite_unlock_notify
@@ -122,6 +123,7 @@ steps:
     image: golang:1.13
     commands:
       - make test
+      - make test-check
     environment:
       GOPROXY: off
       TAGS: bindata sqlite sqlite_unlock_notify
@@ -150,6 +152,7 @@ steps:
     image: golang:1.13
     commands:
       - make test
+      - make test-check
     environment:
       GOPROXY: off
       TAGS: bindata
diff --git a/Makefile b/Makefile
index 146abf5d0..1b60997d0 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,20 @@ fmt-check:
 .PHONY: test
 test:
        GO111MODULE=on $(GO) test -mod=vendor -tags='sqlite sqlite_unlock_notify' $(PACKAGES)
+       @echo "\n==>\033[32m Ok\033[m\n"
+
+.PHONY: test-check
+test-check:
+       @echo "Checking if tests have changed the source tree...";
+       @diff=$$(git status -s); \
+       if [ -n "$$diff" ]; then \
+               echo "make test has changed files in the source tree:"; \
+               echo "$${diff}"; \
+               echo "You should change the tests to create these files in a temporary directory."; \
+               echo "Do not simply add these files to .gitignore"; \
+               exit 1; \
+       fi;
+       @echo "==>\033[32m Ok\033[m\n";
 
 .PHONY: test\#%
 test\#%:

@zeripath
Copy link
Contributor Author

I guess it just relies on us being eagle-eyed for changes to .gitignore?

@GiteaBot GiteaBot 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 Dec 28, 2019
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@8841732). Click here to learn what that means.
The diff coverage is 40%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #9515   +/-   ##
========================================
  Coverage          ?   41.6%           
========================================
  Files             ?     573           
  Lines             ?   75679           
  Branches          ?       0           
========================================
  Hits              ?   31487           
  Misses            ?   40280           
  Partials          ?    3912
Impacted Files Coverage Δ
modules/indexer/issues/bleve.go 66.07% <40%> (ø)

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 8841732...b75266f. Read the comment docs.

@lunny lunny merged commit 55cd33e into go-gitea:master Dec 28, 2019
@zeripath zeripath deleted the stop-making-bleve.indexes branch December 28, 2019 23:47
@zeripath zeripath mentioned this pull request Mar 28, 2020
5 tasks
@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.

make test-sqlite create not ignored dir
5 participants