Skip to content

Conversation

@jkurdek
Copy link
Contributor

@jkurdek jkurdek commented Jul 6, 2022

The test generated warnings which were not checked. The XML file was pointing to the wrong assembly and the linker raised IL2072 warning. Instead, the test verified the suppression of warning IL2067, which was not raised by the linker in the first place.

@vitek-karas

@jkurdek jkurdek requested a review from marek-safar as a code owner July 6, 2022 15:52
@dnfadmin
Copy link

dnfadmin commented Jul 6, 2022

CLA assistant check
All CLA requirements met.

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Great - thanks a lot!

@vitek-karas vitek-karas requested a review from tlakollo July 6, 2022 18:05
@vitek-karas vitek-karas merged commit 2898eba into dotnet:main Jul 7, 2022
<?xml version="1.0" encoding="utf-8" ?>
<linker xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../../src/ILLink.Shared/ILLink.LinkAttributes.xsd">
<assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null">
<assembly fullname="*">
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels weird that we have to use star. There is plenty of other XML files for descriptors and XML attributes that use this same method.

Copy link
Member

Choose a reason for hiding this comment

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

I suggested * as it's robust against changes in the framework. Currently the attribute is in CoreLib, so we could have put CoreLib there instead.

My theory is that this test was added before the attribute was added to the framework, so we declared it in the test itself. Later on once we added the attribute to the framework we removed it from the test, but the XML didn't change. And because the test was already broken (was looking for the wrong warning) it didn't fail.

agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
The test generated warnings which were not checked. The XML file was pointing to the wrong assembly and the linker raised IL2072 warning. Instead, the test verified the suppression of warning IL2067, which was not raised by the linker in the first place.

Co-authored-by: Jeremi Kurdek <[email protected]>

Commit migrated from dotnet/linker@2898eba
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.

4 participants