Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Workaround XUnit FileLoadException issues #20644

Merged
merged 5 commits into from
Oct 31, 2018
Merged

Workaround XUnit FileLoadException issues #20644

merged 5 commits into from
Oct 31, 2018

Conversation

echesakov
Copy link

@echesakov echesakov commented Oct 26, 2018

@echesakov echesakov added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 26, 2018
@echesakov echesakov closed this Oct 28, 2018
@echesakov echesakov reopened this Oct 28, 2018
@echesakov
Copy link
Author

java.nio.channels.ClosedChannelException

@dotnet-bot test Windows_NT arm Cross Debug Innerloop Build
@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test

@echesakov
Copy link
Author

java.nio.channels.ClosedChannelException again

@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test

@echesakov
Copy link
Author

java.nio.channels.ClosedChannelException again

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

@echesakov
Copy link
Author

java.nio.channels.ClosedChannelException again
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

@echesakov
Copy link
Author

java.nio.channels.ClosedChannelException again in tst job

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked no_tiered_compilation_innerloop Build and Test

@echesakov echesakov changed the title [NO MERGE] Test Only Workaround XUnit FileLoadException issues Oct 29, 2018
@echesakov echesakov removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 29, 2018
@echesakov
Copy link
Author

This one is got stuck

test Ubuntu16.04 arm64 Cross Checked Innerloop Build and Test

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

This is a very unfortunate workaround.

I would like to see a big comment block in runtest.py stating why this is being done, with description of what is in the replacement assembly, when we can get rid of this code, etc. I'd like to see a comment somewhere that specifies the xunit dependency that refers to this workaround, so if the xunit dependency changes we could remember to remove the workaround.

@echesakov
Copy link
Author

@BruceForstall I added the comment, can you please take a look?

Copy link

@jashook jashook left a comment

Choose a reason for hiding this comment

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

Have a few nit comments.

tests/runtest.py Outdated
print("Download and overwrite xunit.console.dll in Core_Root")

try:
import urllib.request
Copy link

@jashook jashook Oct 30, 2018

Choose a reason for hiding this comment

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

I would much rather prefer this be wrapped in a statement checking the version like: if sys.version_info < (3,0):, rather then catching the exception. We will eventually need to remove these python2 isms and this setup makes that work more difficult.

Copy link

Choose a reason for hiding this comment

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

I would suggest this import be conditional at the header of the file. After the last from import add something like:

# Version specific imports

if sys.version_info < (3,0):
    import urllib.request
else:
    import urllib

Copy link

Choose a reason for hiding this comment

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

Then you can add the if here as well. I am fine with changing this to a method call is_python3.

if sys.version_info < (3,0):
    urlretrieve = urllib.request.urlretrieve
else:
    urlretrieve = urllib.urlretrieve

tests/runtest.py Outdated
ziparch.extractall(core_root)

os.remove(zipfilename)

Copy link

Choose a reason for hiding this comment

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

Nit, extra newline

with zipfile.ZipFile(zipfilename,"r") as ziparch:
ziparch.extractall(core_root)

os.remove(zipfilename)
Copy link

Choose a reason for hiding this comment

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

Nit: Add an assert that !os.path.isfile(zipfilename)

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Excellent, detailed comment. Thanks!

@echesakov
Copy link
Author

java.nio.channels.ClosedChannelException

test Windows_NT arm Cross Checked Innerloop Build and Test
test Windows_NT arm64 Cross Checked Innerloop Build and Test

@echesakov
Copy link
Author

java.nio.channels.ClosedChannelException

test Windows_NT arm Cross Checked Innerloop Build and Test

@echesakov
Copy link
Author

test Windows_NT arm Cross Checked Innerloop Build and Test

@echesakov echesakov merged commit 4bcbb70 into dotnet:master Oct 31, 2018
@echesakov echesakov deleted the UpdateXunitConsoleDll branch November 1, 2018 03:01
kouvel pushed a commit to kouvel/coreclr that referenced this pull request Nov 3, 2018
In order to mitigate System.IO.FileLoadException we built our own xunit.console.dll with ConcurrentDictionary used for DependencyContextAssemblyCache.managedAssemblyCache instead of Dictionary and use this instead of the one pulled from NuGet.
A-And pushed a commit to A-And/coreclr that referenced this pull request Nov 20, 2018
In order to mitigate System.IO.FileLoadException we built our own xunit.console.dll with ConcurrentDictionary used for DependencyContextAssemblyCache.managedAssemblyCache instead of Dictionary and use this instead of the one pulled from NuGet.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
In order to mitigate System.IO.FileLoadException we built our own xunit.console.dll with ConcurrentDictionary used for DependencyContextAssemblyCache.managedAssemblyCache instead of Dictionary and use this instead of the one pulled from NuGet.


Commit migrated from dotnet/coreclr@4bcbb70
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants