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

Ensure that the detected charset order is set in chardet test #12574

Merged
merged 1 commit into from Aug 23, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Aug 23, 2020

TestToUTF8WithFallback is the cause of recurrent spurious test failures
even despite code to set the detected charset order.

The reason why this happens is because the preferred detected charset order
is not being initialised for these tests.

This PR simply ensures that this is set at the start of each test and would
allow different tests to be written to allow differing orders.

Replaces #12571
Close #12571
Fix #12569

Signed-off-by: Andrew Thornton art27@cantab.net

TestToUTF8WithFallback is the cause of recurrent spurious test failures
even despite code to set the detected charset order.

The reason why this happens is because the preferred detected charset order
is not being initialised for these tests.

This PR simply ensures that this is set at the start of each test and would
allow different tests to be written to allow differing orders.

Replaces go-gitea#12571
Close go-gitea#12571

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile backport/v1.12 labels Aug 23, 2020
@zeripath zeripath added this to the 1.13.0 milestone Aug 23, 2020
@zeripath
Copy link
Contributor Author

I've made this backport so that we can prevent the bug from occurring during building of 1.12 - we can skip-changelog it.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 23, 2020
@zeripath zeripath added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Aug 23, 2020
@zeripath
Copy link
Contributor Author

Now I am sure that #11621 (at least temporarily) fixed this issue - I repeatedly ran CI builds on that and did not get a failure.

If I am correct that it did fix this - there was likely a change at some point that caused the unit tests to stop calling modules/setting.newRepository() (which is on the whole a good thing) but would have caused this issue to reappear.

@silverwind
Copy link
Member

Seems to work. I could not get any more failure in over 100 runs. So the non-determinism was a just a race between test cases after all?

$ while true; do go test -count=1 -run TestToUTF8 ./modules/charset; done
ok    code.gitea.io/gitea/modules/charset 0.036s
ok    code.gitea.io/gitea/modules/charset 0.035s
ok    code.gitea.io/gitea/modules/charset 0.035s
ok    code.gitea.io/gitea/modules/charset 0.039s
ok    code.gitea.io/gitea/modules/charset 0.037s
ok    code.gitea.io/gitea/modules/charset 0.038s
ok    code.gitea.io/gitea/modules/charset 0.037s
ok    code.gitea.io/gitea/modules/charset 0.038s
ok    code.gitea.io/gitea/modules/charset 0.038s
ok    code.gitea.io/gitea/modules/charset 0.037s
ok    code.gitea.io/gitea/modules/charset 0.038s
ok    code.gitea.io/gitea/modules/charset 0.040s

@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 Aug 23, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #12574 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12574      +/-   ##
==========================================
- Coverage   43.43%   43.42%   -0.01%     
==========================================
  Files         643      643              
  Lines       71165    71165              
==========================================
- Hits        30910    30904       -6     
- Misses      35246    35252       +6     
  Partials     5009     5009              
Impacted Files Coverage Δ
modules/git/utils.go 73.77% <0.00%> (-3.28%) ⬇️
services/pull/check.go 47.69% <0.00%> (-2.31%) ⬇️
modules/log/event.go 57.54% <0.00%> (-1.89%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.54%) ⬇️
models/error.go 34.81% <0.00%> (-0.52%) ⬇️
modules/git/repo.go 49.23% <0.00%> (-0.51%) ⬇️
models/issue_comment.go 53.75% <0.00%> (-0.16%) ⬇️
services/pull/pull.go 42.03% <0.00%> (+0.46%) ⬆️
modules/indexer/stats/db.go 60.86% <0.00%> (+17.39%) ⬆️
modules/indexer/stats/queue.go 76.47% <0.00%> (+23.52%) ⬆️

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 2026d88...1c5366e. Read the comment docs.

@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 Aug 23, 2020
@zeripath zeripath merged commit e429c11 into go-gitea:master Aug 23, 2020
@zeripath zeripath deleted the fix-chardet-test branch August 23, 2020 13:15
zeripath added a commit to zeripath/gitea that referenced this pull request Aug 23, 2020
…ea#12574)

Backport go-gitea#12574

TestToUTF8WithFallback is the cause of recurrent spurious test failures
even despite code to set the detected charset order.

The reason why this happens is because the preferred detected charset order
is not being initialised for these tests.

This PR simply ensures that this is set at the start of each test and would
allow different tests to be written to allow differing orders.

Replaces go-gitea#12571
Close go-gitea#12571

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

Not backportable to 1.12 because #11621 is not in 1.12

@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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test TestToUTF8
5 participants