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

ObjectModelHelpers uses loggers improperly #6521

Closed
KirillOsenkov opened this issue Jun 4, 2021 · 2 comments · Fixed by #8327
Closed

ObjectModelHelpers uses loggers improperly #6521

KirillOsenkov opened this issue Jun 4, 2021 · 2 comments · Fixed by #8327
Assignees
Labels
Area: Logging Good First Issue Self-contained issues good for first-time contributors. Iteration:2023February testing triaged
Milestone

Comments

@KirillOsenkov
Copy link
Member

Need to refactor ObjectModelHelpers.BuildProjectExpectSuccess to properly use loggers.

Issues I've noticed:

  1. loggers are not passed to evaluation, so we drop all evaluation messages
  2. a ProjectCollection created is not disposed, so when loggers ARE passed to evaluation they are not shutdown
  3. it's unclear whether to pass loggers to Build or just rely on them being in the ProjectCollection
@KirillOsenkov
Copy link
Member Author

Turns out that we are missing out on coverage because we're only passing the MockLogger in most tests, and it's not diagnostic, so we're not exercising Diagnostic codepaths, such as LogTaskInputs.

For instance, this test:

public void NullMetadataOnLegacyOutputItems_InlineTask()

will fail if you change the verbosity to diagnostic, or explicitly enable LogTaskInputs.

@KirillOsenkov
Copy link
Member Author

Ughhh, you guys, change the MockLogger Verbosity to Diagnostic and weep.

get => LoggerVerbosity.Normal;

KirillOsenkov added a commit that referenced this issue Jun 8, 2021
The feature the test is testing is broken, but the test passes because it doesn't specify diag verbosity for its logger.

We will only log task outputs with diag verbosity.

Issue #6521 is tracking.
@rainersigwald rainersigwald added Area: Logging and removed needs-triage Have yet to determine what bucket this goes in. labels Jun 9, 2021
@rainersigwald rainersigwald added this to the Backlog milestone Jun 9, 2021
KirillOsenkov added a commit that referenced this issue Jun 15, 2021
The feature the test is testing is broken, but the test passes because it doesn't specify diag verbosity for its logger.

We will only log task outputs with diag verbosity.

Issue #6521 is tracking.
@KirillOsenkov KirillOsenkov added the Good First Issue Self-contained issues good for first-time contributors. label Oct 31, 2022
@vlada-shubina vlada-shubina self-assigned this Jan 19, 2023
vlada-shubina added a commit to vlada-shubina/msbuild that referenced this issue Jan 20, 2023
vlada-shubina added a commit to vlada-shubina/msbuild that referenced this issue Jan 26, 2023
vlada-shubina added a commit to vlada-shubina/msbuild that referenced this issue Feb 7, 2023
JaynieBai pushed a commit that referenced this issue Feb 8, 2023
Fixes #6521

Context
Did suggested refactoring in the issue.
NullMetadataOnLegacyOutputItems_InlineTask won't be fixed as it tests Deprecated code, and change is needed there.

Changes Made
loggers are now passed to evaluation
ability to set logger verbosity to MockLogger and when using BuildProjectExpectSuccess and BuildProjectExpectFailure
same loggers are reused during build when using BuildProjectExpectSuccess and BuildProjectExpectFailure
registering logging is now done before the project is evaluated
improved doc and formatting in changed files
disposing of ProjectCollection in BuildProjectExpectSuccess and BuildProjectExpectFailure
Testing
Testing only changes

Notes
There are other calls to ObjectModelHelpers.CreateInMemoryProject which do not dispose ProjectCollection. Those needs to be fixed separately.
dotnet-bot pushed a commit to dotnet/dotnet that referenced this issue Feb 8, 2023
Diff: https://github.com/dotnet/msbuild/compare/446f42e6d1478acb44faec787f90f57f38c08b73..51df47643a8ee2715ac67fab8d652b25be070cd2

From: dotnet/msbuild@446f42e
To: dotnet/msbuild@51df476

Commits:
  - dotnet/msbuild#6521 `ObjectModelHelpers` refactoring (#8327)
    dotnet/msbuild@c3541c6
  - Add nodes orchestration doc (#8383)
    dotnet/msbuild@a57cc6d
  - Add a comment to all sources under src/Deprecated (#8380)
    dotnet/msbuild@8afdfd0
  - Enable temporary comm logging for CanShutdownServerProcess test (#8378)
    dotnet/msbuild@3d5ca47
  - Add .DS_Store to .gitignore (#8377)
    dotnet/msbuild@70ac2f2
  - Merge pull request #8323 from dotnet-maestro-bot/merge/vs17.4-to-main
    dotnet/msbuild@1085e21
  - Introduce backport GHA (#8368)
    dotnet/msbuild@03dae90
  - [main] Update dependencies from dotnet/roslyn (#8338)
    dotnet/msbuild@2c2ea7b
  - Bump Microsoft.CodeAnalysis.BannedApiAnalyzers in /eng/dependabot (#8335)
    dotnet/msbuild@f0f9c50
  - Add ability to create temp mapped drive for integration tests (#8366)
    dotnet/msbuild@51df476

[[ commit created by automation ]]
dotnet-bot pushed a commit to dotnet/dotnet that referenced this issue Feb 16, 2023
Diff: https://github.com/dotnet/msbuild/compare/fc3ab4c5e2a486abb8fc66aede7ec8e3eb91fe08..dfd8f413a80cd0865f968b2c0ad9b09c0df8c430

From: dotnet/msbuild@fc3ab4c
To: dotnet/msbuild@dfd8f41

Commits:
  - [FancyLogger] Show link to project outputs (#8324)
    dotnet/msbuild@76215a0
  - [LiveLogger] Add code to high priority messages (#8397)
    dotnet/msbuild@e1b3bf9
  - Added README.md and updated the table of contents in `documentation` folder (#8390)
    dotnet/msbuild@e073a39
  - Support `SkipNonexistentTargets` in project reference target protocol (#8330)
    dotnet/msbuild@446f42e
  - dotnet/msbuild#6521 `ObjectModelHelpers` refactoring (#8327)
    dotnet/msbuild@c3541c6
  - Add nodes orchestration doc (#8383)
    dotnet/msbuild@a57cc6d
  - Add a comment to all sources under src/Deprecated (#8380)
    dotnet/msbuild@8afdfd0
  - Enable temporary comm logging for CanShutdownServerProcess test (#8378)
    dotnet/msbuild@3d5ca47
  - Add .DS_Store to .gitignore (#8377)
    dotnet/msbuild@70ac2f2
  - Merge pull request #8323 from dotnet-maestro-bot/merge/vs17.4-to-main
    dotnet/msbuild@1085e21
  - Introduce backport GHA (#8368)
    dotnet/msbuild@03dae90
  - [main] Update dependencies from dotnet/roslyn (#8338)
    dotnet/msbuild@2c2ea7b
  - Bump Microsoft.CodeAnalysis.BannedApiAnalyzers in /eng/dependabot (#8335)
    dotnet/msbuild@f0f9c50
  - Add ability to create temp mapped drive for integration tests (#8366)
    dotnet/msbuild@51df476
  - [LiveLogger] First tweaks to UI (#8385)
    dotnet/msbuild@81877bd
  - New Trace overloads without array allocation and boxing (#8345)
    dotnet/msbuild@cfefebd
  - Skip symlink tests when symlinks cannot be created (#8328)
    dotnet/msbuild@bace714
  - Update links referencing git branches from master -> main (#8437)
    dotnet/msbuild@dba9b23
  - Store desired color for top color (#8427)
    dotnet/msbuild@b8c160e
  - switched to YAML github tempaltes (#8423)
    dotnet/msbuild@2b4c585
  - Enable CA1852: Seal internal types (#8389)
    dotnet/msbuild@d131702
  - Warn about unused WriteOnlyWhenDifferent attribute (#8371)
    dotnet/msbuild@70c8837
  - Fix temp file filtering in FileTracker (#8351)
    dotnet/msbuild@03de075
  - [main] Update dependencies from dotnet/roslyn (#8442)
    dotnet/msbuild@dfd8f41

[[ commit created by automation ]]
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Logging Good First Issue Self-contained issues good for first-time contributors. Iteration:2023February testing triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants