SystemIOFile Adapter #550

daveMueller · 2019-09-16T22:43:40Z

closes #543

update fork

Update fork

daveMueller · 2019-09-16T23:19:41Z

Can anyone help out here? Can't really figure out why one test is failing on the build server. I can execute them all locally without failing.

MarcoRossignoli · 2019-09-17T07:20:40Z

I think you need to align to master yesterday I changed one behaviour of logs and amended tests #548

MarcoRossignoli · 2019-09-17T07:31:26Z

@daveMueller for the moment I would avoid to update collectors code, because could live alone(I mean the infrastructure to load coverage core), so could you revert all changes under coverlet.collector project?
I think is premature here for now(consolidate under one interface different context).
Sorry I didn't specify that on issue 😞
The idea is to make core more testable...collectors are born recently and follow that pattern yet.

src/coverlet.core/Abstracts/IFileSystem.cs

src/coverlet.core/Coverage.cs

src/coverlet.core/Helpers/FileSystem.cs

src/coverlet.core/Helpers/InstrumentationHelper.cs

MarcoRossignoli · 2019-09-17T07:57:09Z

src/coverlet.msbuild.tasks/CoverageResultTask.cs

                {
                    _logger.LogError("Result of instrumentation task not found");
                    return false;
                }

-                var coverage = new Coverage(CoveragePrepareResult.Deserialize(new FileStream(InstrumenterState.ItemSpec, FileMode.Open)), this._logger, (IInstrumentationHelper)DependencyInjection.Current.GetService(typeof(IInstrumentationHelper)));
+                var coverage = new Coverage(CoveragePrepareResult.Deserialize(new FileStream(InstrumenterState.ItemSpec, FileMode.Open)), this._logger, (IInstrumentationHelper)DependencyInjection.Current.GetService(typeof(IInstrumentationHelper)), fileSystem);


Why not also for new FileStream(InstrumenterState.ItemSpec, FileMode.Open)?
And we should surround by a using to close handle asap(my past error).

src/coverlet.msbuild.tasks/InstrumentationTask.cs

src/coverlet.core/Helpers/FileSystem.cs

src/coverlet.msbuild.tasks/InstrumentationTask.cs

MarcoRossignoli · 2019-09-17T08:35:35Z

test/coverlet.core.tests/CoverageTests.cs

@@ -30,7 +30,7 @@ public void TestCoverage()

            // TODO: Find a way to mimick hits

-            var coverage = new Coverage(Path.Combine(directory.FullName, Path.GetFileName(module)), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), false, false, string.Empty, false, _mockLogger.Object, _instrumentationHelper);
+            var coverage = new Coverage(Path.Combine(directory.FullName, Path.GetFileName(module)), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), false, false, string.Empty, false, _mockLogger.Object, new Mock<IFileSystem>().Object, _instrumentationHelper);


move mock instantiation of mock under private readonly Mock<ILogger> _mockLogger = new Mock<ILogger>(); and reuse

src/coverlet.core/Abstracts/IFileSystem.cs

src/coverlet.core/Helpers/FileSystem.cs

src/coverlet.core/Helpers/InstrumentationHelper.cs

MarcoRossignoli · 2019-09-18T08:06:33Z

src/coverlet.core/Abstracts/IFileSystem.cs

+
+        Stream OpenRead(string path);
+
+        void Copy(string sourceFileName, string destFileName);


why?Where is used?

Sorry this was used in the coverlet.collector and I missed to revert it.

src/coverlet.core/Abstracts/IFileSystem.cs

src/coverlet.core/Helpers/FileSystem.cs

MarcoRossignoli · 2019-09-18T08:10:04Z

src/coverlet.msbuild.tasks/CoverageResultTask.cs

                {
                    _logger.LogError("Result of instrumentation task not found");
                    return false;
                }

-                var coverage = new Coverage(CoveragePrepareResult.Deserialize(new FileStream(InstrumenterState.ItemSpec, FileMode.Open)), this._logger, (IInstrumentationHelper)DependencyInjection.Current.GetService(typeof(IInstrumentationHelper)));
+                var coverage = new Coverage(CoveragePrepareResult.Deserialize(new FileStream(InstrumenterState.ItemSpec, FileMode.Open)), this._logger, fileSystem, (IInstrumentationHelper)DependencyInjection.Current.GetService(typeof(IInstrumentationHelper)));


also for new FileStream(InstrumenterState.ItemSpec, FileMode.Open) and surround by using please(my past error)

Ok I changed it but I don't know how to surround this by a using.

something like

Coverage coverage = null; using(Stream instrumenterStateStream = fileSystem.NewFileStream(InstrumenterState.ItemSpec, FileMode.Open)) { coverage = new Coverage(CoveragePrepareResult.Deserialize(instrumenterStateStream), this._logger, fileSystem,... }

MarcoRossignoli · 2019-09-18T08:11:17Z

src/coverlet.msbuild.tasks/InstrumentationTask.cs

+                    _logger,
+                    (IFileSystem) DependencyInjection.Current.GetService(typeof(IFileSystem)),
+                    (IInstrumentationHelper) DependencyInjection.Current.GetService(typeof(IInstrumentationHelper)));
+
                CoveragePrepareResult prepareResult = coverage.PrepareModules();
                InstrumenterState = new TaskItem(System.IO.Path.GetTempFileName());
                using (var instrumentedStateFile = new FileStream(InstrumenterState.ItemSpec, FileMode.Open, FileAccess.Write))


also for new FileStream(InstrumenterState.ItemSpec, FileMode.Open, FileAccess.Write) and surround by using please(my past error)

MarcoRossignoli · 2019-09-18T08:15:05Z

test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs

            string coverletLib = Directory.GetFiles(Directory.GetCurrentDirectory(), "coverlet.core.dll").First();
+            var instrumentationHelperMock = new Mock<IInstrumentationHelper>();


Why you've changed this test?
Here I want to test InstrumentationHelper logic and I have to mock only file system to simulate "file not found".
Revert please.

I don't exactly know what you mean or how I can make this test run again by only mocking the FileSystem. When I revert the test to how it was before the test fails. This is because I now also use the FileSystem adapter for the streams. Thus I also must setup e.g. _fileSystem.OpenRead()....

And wouldn't it be better to test the InstrumentationHelper logic in the class InstrumentationHelperTests.

And wouldn't it be better to test the InstrumentationHelper logic in the class InstrumentationHelperTests.

Here we have a bit of integration...I mean we need to test the interaction between InstrumentationHelper and Instrumenter.
You should do partial mock of concrete implementation and redefine only Exists something like
https://stackoverflow.com/questions/4769928/using-moq-to-mock-only-some-methods

var mock = new Mock<YourTestClass>(); mock.CallBase = true; mock.Setup(x => x.GetCurrentUser()).Returns(lUnauthorizedUser); mockedTest.Object.MyMethod();

In this test we need to simulate to find pdb file but return false when we try to understand if sources are present on local hard drive, so we did it with coverlet.core.dll to avoid new dependency and we need to return false also if we have sources in local.

Ok done. Thanks for the information with the partial mock. 👍

MarcoRossignoli · 2019-09-18T08:21:27Z

@MarcoRossignoli I have a hard time fixing those two test. I pass a real instance of FileSystem() to Coverage ctor. When the test then executes coverage.GetCoverageResult() the field _fileSystem is null.

@daveMueller because you added two new constructor to Coverage class that are unuseful.
Coverage class has got 2 contructor used in two phases:

Instrumentation
Calculate coverage using hit files at the end of tests.

These two constructor accept different set of parameters but both use file system, so we need only to pass new service.

daveMueller

These two constructor accept different set of parameters but both use file system, so we need only to pass new service.

Yes sorry. I felt really ashamed when I saw your commit this morning. I was probably way to tired yesterday night. Sorry.

MarcoRossignoli · 2019-09-18T11:43:00Z

Yes sorry. I felt really ashamed when I saw your commit this morning. I was probably way to tired yesterday night. Sorry.

You don't have to apologize, you're great contributor!We're all human and happen also to me when I work in the night...actually also in the morning 😄

YOUR HELP HERE IS VERY WELCOME!

MarcoRossignoli · 2019-09-18T11:54:23Z

src/coverlet.core/Instrumentation/Instrumenter.cs

@@ -136,7 +136,9 @@ public InstrumenterResult Instrument()

        private void InstrumentModule()
        {
-            using (var stream = new FileStream(_module, FileMode.Open, FileAccess.ReadWrite))
+            var fileSystem = (IFileSystem)DependencyInjection.Current.GetService(typeof(IFileSystem));


We shoud avoid DependencyInjection reference in code...we need to inject throught contructor and update al callers.
The idea is to have testabililty and the possibility to "change every implementation" also for DependencyInjection(the container).
So the principle here is to have a "container" where we register dependences, and in our case is DependencyInjection class.
And we inject every dependence in constructors...the only place where we directly use DependencyInjection class is when we don't have an "instance", but we removed all statics for this reason(testability).
DependencyInjection is only the place where we chose the implementation and keep them aligned in every driver(collector, msbuild, console).

MarcoRossignoli · 2019-09-18T12:01:16Z

src/coverlet.core/Instrumentation/ModuleTrackerTemplate.cs

@@ -72,6 +73,8 @@ public static void RecordSingleHit(int hitLocationIndex)

        public static void UnloadModule(object sender, EventArgs e)
        {
+            var fileSystem = (IFileSystem)DependencyInjection.Current.GetService(typeof(IFileSystem));


Revert update in this file please.
This is a key piece of instrumentation, this class is injected inside users dll so we have to try to pollute less possible and use "runtime" only class.
In this case is not a problem..I mean core lib and instrumented lib are in same directory so we don't have issue with load...but I don't know how this evolve in future so I prefer keep this class(tracker) "light".

FYI I'm trying to get logs also for it #553
And the idea si to enable instrumentation throught enviroment variables, this is the only static part needed today.

Ok done, reverted the changes.

MarcoRossignoli · 2019-09-18T12:06:06Z

src/coverlet.msbuild.tasks/CoverageResultTask.cs

                {
                    _logger.LogError("Result of instrumentation task not found");
                    return false;
                }

-                var coverage = new Coverage(CoveragePrepareResult.Deserialize(new FileStream(InstrumenterState.ItemSpec, FileMode.Open)), this._logger, (IInstrumentationHelper)DependencyInjection.Current.GetService(typeof(IInstrumentationHelper)));
+                var coverage = new Coverage(


format inline please here the contructor parameter is not so bad, I mean keep old formatting here

src/coverlet.core/Helpers/FileSystem.cs

MarcoRossignoli · 2019-09-20T12:51:17Z

src/coverlet.core/Coverage.cs

            _instrumentationHelper = instrumentationHelper;

            _identifier = Guid.NewGuid().ToString();
            _results = new List<InstrumenterResult>();
        }

-        public Coverage(CoveragePrepareResult prepareResult, ILogger logger, IInstrumentationHelper instrumentationHelper)
+        public Coverage(CoveragePrepareResult prepareResult, ILogger logger, IFileSystem fileSystem, IInstrumentationHelper instrumentationHelper)


please keep IFileSystem as last parameter we'll add new service at the end of the signature as a rule of thumb

MarcoRossignoli · 2019-09-20T12:52:25Z

src/coverlet.msbuild.tasks/CoverageResultTask.cs

+                Coverage coverage = null;
+                using (Stream instrumenterStateStream = fileSystem.NewFileStream(InstrumenterState.ItemSpec, FileMode.Open))
+                {
+                    coverage = new Coverage(CoveragePrepareResult.Deserialize(instrumenterStateStream), this._logger, fileSystem, (IInstrumentationHelper)DependencyInjection.Current.GetService(typeof(IInstrumentationHelper)));


file system service for last param due to above comment

MarcoRossignoli · 2019-09-20T12:52:55Z

test/coverlet.core.tests/CoverageTests.cs

@@ -30,7 +31,7 @@ public void TestCoverage()

            // TODO: Find a way to mimick hits

-            var coverage = new Coverage(Path.Combine(directory.FullName, Path.GetFileName(module)), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), false, false, string.Empty, false, _mockLogger.Object, _instrumentationHelper);
+            var coverage = new Coverage(Path.Combine(directory.FullName, Path.GetFileName(module)), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), false, false, string.Empty, false, _mockLogger.Object, _mockFileSystem.Object, _instrumentationHelper);


fix constructors param order here and below

MarcoRossignoli · 2019-09-20T12:53:48Z

test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs

-            InstrumentationHelper instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), mockFileSystem.Object);
+            Mock<FileSystem> partialMockFileSystem = new Mock<FileSystem>();
+            partialMockFileSystem.CallBase = true;
+            partialMockFileSystem.Setup(fs => fs.Exists(It.IsAny<string>())).Returns((string path) =>


MarcoRossignoli

LGTM Thank's Dave!

daveMueller added 14 commits August 18, 2019 01:49

Merge pull request #1 from tonerdo/master

a6e0687

update fork

Merge pull request #2 from tonerdo/master

84c0aff

Update fork

Merge pull request #3 from tonerdo/master

5b4e316

Update fork

Merge pull request #4 from tonerdo/master

45075e1

Update fork

extended SystemIoFileAdapter and interface

23aeb1c

removed IFileHelper and used IFileSystem instead + refactorings

7ca75f2

File adapter on Program.cs

71af4de

File adapter on Coverage.cs + refactorings

8ec57c1

File adapter for CoverageResultTask + refactorings

8a4a1f8

File adapter on InstumenterHelper

3875965

File adapter on RemoteExecutor

c889d15

changes to CoverletCoverageDataCollectorTests

202f965

changes to collector

c58508b

changed Test

5c11795

daveMueller changed the title ~~543 system io file adapter~~ SystemIOFile Adapter Sep 16, 2019

MarcoRossignoli requested a review from tonerdo September 17, 2019 07:20

MarcoRossignoli added the enhancement General enhancement request label Sep 17, 2019

MarcoRossignoli requested a review from petli September 17, 2019 07:34

MarcoRossignoli reviewed Sep 17, 2019

View reviewed changes

src/coverlet.core/Abstracts/IFileSystem.cs Show resolved Hide resolved

MarcoRossignoli reviewed Sep 17, 2019

View reviewed changes

src/coverlet.core/Coverage.cs Show resolved Hide resolved

MarcoRossignoli reviewed Sep 17, 2019

View reviewed changes

src/coverlet.core/Helpers/FileSystem.cs Show resolved Hide resolved

MarcoRossignoli reviewed Sep 17, 2019

View reviewed changes

src/coverlet.core/Helpers/InstrumentationHelper.cs Show resolved Hide resolved

MarcoRossignoli reviewed Sep 17, 2019

View reviewed changes

src/coverlet.msbuild.tasks/InstrumentationTask.cs Outdated Show resolved Hide resolved

MarcoRossignoli reviewed Sep 17, 2019

View reviewed changes

src/coverlet.core/Helpers/FileSystem.cs Outdated Show resolved Hide resolved

MarcoRossignoli reviewed Sep 17, 2019

View reviewed changes

src/coverlet.msbuild.tasks/InstrumentationTask.cs Outdated Show resolved Hide resolved

MarcoRossignoli reviewed Sep 17, 2019

View reviewed changes