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

Tests: Protect RefRepos with mutex before reset #9856

Merged
merged 2 commits into from Feb 13, 2022

Conversation

gerhardol
Copy link
Member

@gerhardol gerhardol commented Feb 4, 2022

Proposed changes

Many tests (one out of four?) have failed recently:

LibGit2Sharp.LockedFileException : the index is locked; this might be due to a concurrent or crashed process

https://ci.appveyor.com/project/gitextensions/gitextensions/builds/42433206

These issues occur when a ReferenceRepository is to be reset before next test case.
There seem to be some reuse of repos, parallel tests or repos that happen to use the same path.

The reason for the failure is unknown, but the reset is just a way to speed up the test and should not fail the test case.
So a try-catch to create a new repo is used.

Two tests became unstable after this, ignoring.
I have no clue why they occur after these changes, seem to be related to .net5

   at GitExtensions.UITests.CommandsDialogs.FormEditorTests.Should_preserve_encoding(Encoding encoding) in C:\projects\gitextensions\IntegrationTests\UI.IntegrationTests\CommandsDialogs\FormEditorTests.cs:line 87
   at GitExtensions.UITests.CommandsDialogs.FormEditorTests.Should_preserve_encoding_utf8_bom() in C:\projects\gitextensions\IntegrationTests\UI.IntegrationTests\CommandsDialogs\FormEditorTests.cs:line 82
  Expected: <System.Text.UTF8Encoding>
  But was:  <System.Text.UTF8Encoding+UTF8EncodingSealed>

Edit: The initial version of this PR used a mutex to protect simultaneous changes but that did not prevent all changes.
That solution was inspired by https://github.com/GitTools/GitVersion/pull/2669/files

Test methodology

Rerun AppVeyor tests many times (manually).

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned gerhardol Feb 4, 2022
@gerhardol gerhardol marked this pull request as draft February 4, 2022 23:41
@gerhardol gerhardol marked this pull request as ready for review February 5, 2022 21:54
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

+1

@@ -79,12 +70,14 @@ public void HasChanges_updated_correctly()
}
}

[Ignore("Unstable UTF8EncodingSealed result")]
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps ignore +UTF8EncodingSealed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I really would like to know why this appear at this point, also when rolling back the changes...
What exact wording do you want?

Copy link
Member

Choose a reason for hiding this comment

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

(Too short as too often) I meant to make the test accept both results.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not see a way to create this encoding.
Would a regex match on the output encoding text be OK?

Copy link
Member

Choose a reason for hiding this comment

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

I withdraw my proposal because I do not know enough about this topic.
BTW: For #9860, a BOM must be added to the English.xlf.
Perhaps this is caused by different .NET patch levels of different AppVeyor instances.

Copy link
Member Author

Choose a reason for hiding this comment

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

@RussKie Suggestions how to proceed with these tests?
(of course you are welcome to comment on the rest of the PR too...)

Copy link
Member

Choose a reason for hiding this comment

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

BTW: For #9860, a BOM must be added to the English.xlf.
Perhaps this is caused by different .NET patch levels of different AppVeyor instances.

This sounds like dotnet/runtime#63585

@mstv mstv mentioned this pull request Feb 6, 2022
@gerhardol
Copy link
Member Author

Rebased on master as merge of #9805 required this update too.

@RussKie
Copy link
Member

RussKie commented Feb 8, 2022

Two tests became unstable after this, ignoring. I have no clue why they occur after these changes, seem to be related to .net5

   at GitExtensions.UITests.CommandsDialogs.FormEditorTests.Should_preserve_encoding(Encoding encoding) in C:\projects\gitextensions\IntegrationTests\UI.IntegrationTests\CommandsDialogs\FormEditorTests.cs:line 87
   at GitExtensions.UITests.CommandsDialogs.FormEditorTests.Should_preserve_encoding_utf8_bom() in C:\projects\gitextensions\IntegrationTests\UI.IntegrationTests\CommandsDialogs\FormEditorTests.cs:line 82
  Expected: <System.Text.UTF8Encoding>
  But was:  <System.Text.UTF8Encoding+UTF8EncodingSealed>

Is this happening for your locally or on AppVeyor? If it's failing locally, check dotnet/runtime#43295, may be it can give you some clues.

@gerhardol
Copy link
Member Author

Is this happening for your locally or on AppVeyor? If it's failing locally, check dotnet/runtime#43295, may be it can give you some clues.

Both, also when reverting. Maybe AppVeyor images differs, this is seen in #9680 too.
#9856 (comment)

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

Let's take this as it is.
The ignored tests can be re-enabled when the dotnet/runtime bugs have been fixed, keyword UTF8EncodingSealed.

@RussKie
Copy link
Member

RussKie commented Feb 8, 2022

Have you observed utf8 related tests failing on the vs2022 image?

@gerhardol
Copy link
Member Author

Have you observed utf8 related tests failing on the vs2022 image?

I have not used the vs2022 image myself

@mstv
Copy link
Member

mstv commented Feb 8, 2022

Have you observed utf8 related tests failing on the vs2022 image?

Yes, I did but not always, e.g. https://ci.appveyor.com/project/gitextensions/gitextensions/builds/42471108#L370.

@gerhardol
Copy link
Member Author

Can this be merged, it is quite annoying that tests fail so often.

For the disabled tests, it seem to be a coincidence that this occur after this change and I can drop that commit but it seems like it will continue occurring.
We can add an issue about it (that will be closed in some months but at least a reminder...)

@gerhardol gerhardol merged commit dd0a902 into gitextensions:master Feb 13, 2022
@gerhardol gerhardol deleted the feature/libgitsharp-locked branch February 13, 2022 21:53
@ghost ghost added this to the vNext milestone Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants