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.

Sign up for GitHub

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

Jump to bottom

add test for opening and reading from device interface #54673

Merged
merged 7 commits into from
Jun 30, 2021
Merged

add test for opening and reading from device interface #54673

merged 7 commits into from
Jun 30, 2021

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Jun 24, 2021 •
edited
Loading

test for #54143

@adamsitnik adamsitnik added this to the 6.0.0 milestone Jun 24, 2021
@ghost
Copy link

ghost commented Jun 24, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: adamsitnik
Assignees: -
Labels:

area-System.IO
Milestone: 6.0.0

@adamsitnik
Copy link
Member Author

@stephentoub what is your opinion about having tests like this? On one hand we get a very unique test code coverage, but on the other the amount of sys-calls required to setup is very high.

@stephentoub
Copy link
Member

on the other the amount of sys-calls required to setup is very high.

What is the concern with that?

@adamsitnik
Copy link
Member Author

What is the concern with that?

I was afraid that others may perceive it as more complexity than value being added.

@stephentoub
Copy link
Member

It can always be made outerloop, which would be good to do anyway if it's mucking with the machine in some way.

@adamsitnik adamsitnik mentioned this pull request Jun 25, 2021
Open
8 tasks
@adamsitnik
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik adamsitnik closed this Jun 25, 2021
@adamsitnik adamsitnik reopened this Jun 25, 2021
@adamsitnik
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Jun 25, 2021
Closed
jozkee
jozkee approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Otherwise lgtm; thanks, Adam.

src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamConformanceTests.Windows.cs Show resolved Hide resolved
...ries/System.IO.FileSystem/tests/Net5CompatTests/System.IO.FileSystem.Net5Compat.Tests.csproj Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamConformanceTests.Windows.cs Show resolved Hide resolved
carlossanlop
carlossanlop approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

I only have a question about the line removed from the *.csproj. Otherwise, LGTM.

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik adamsitnik merged commit 38df267 into dotnet:main Jun 30, 2021
@adamsitnik adamsitnik deleted the deviceIds branch June 30, 2021 14:57
@adamsitnik adamsitnik mentioned this pull request Jul 1, 2021
Closed
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@danmoseley danmoseley danmoseley left review comments

@jozkee jozkee jozkee approved these changes

@carlossanlop carlossanlop carlossanlop approved these changes

Assignees
No one assigned
Labels
area-System.IO
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@adamsitnik @stephentoub @carlossanlop @danmoseley @jkotas @jozkee