Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Sep 24 21:12
    jonfortescue commented #43109
  • Sep 24 09:40
    nobiit starred dotnet/corefx
  • Sep 24 07:55
    jk2K starred dotnet/corefx
  • Sep 24 07:48
    Aayers-ghw starred dotnet/corefx
  • Sep 23 21:19
    Dotnet-GitSync-Bot labeled #43109
  • Sep 23 21:19
    jonfortescue review_requested #43109
  • Sep 23 21:19
    jonfortescue assigned #43109
  • Sep 23 21:19
    jonfortescue review_requested #43109
  • Sep 23 21:19
    jonfortescue review_requested #43109
  • Sep 23 21:19
    jonfortescue opened #43109
  • Sep 23 03:31
    hczch starred dotnet/corefx
  • Sep 22 23:06
    gmich starred dotnet/corefx
  • Sep 22 09:14
    ryma starred dotnet/corefx
  • Sep 22 09:14
    YsiadH starred dotnet/corefx
  • Sep 21 19:46
    MisterSingh starred dotnet/corefx
  • Sep 20 19:03
    Anipik commented #43102
  • Sep 20 18:57
    zaytsev-victor removed as member
  • Sep 20 18:56
    zakaluka removed as member
  • Sep 20 18:56
    yvanin removed as member
  • Sep 20 18:56
    YoupHulsebos removed as member
CyrusNajmabadi
@CyrusNajmabadi
Why not actually just validate the contents?
Assert.AreEqual(GetHash(result.Output), "BC7C020F61E28086AD90B7E27DA8B21E")
This seems completely not understandable. I have no way to know what the actual expected outcome is. So if somethign breaks all i get is a message saying "XYZ" != "ABC". that doesn't tell me why things are broken or what i would need to do to get a correct hash again.
Mohammad Hamdy Ghanem
@VBAndCs
Think of this as an Integral Test, where I check that the whole generator is working without exception, and the the output has the expected fingerprint.
<TestMethod>
    Public Sub NameValue()
        Dim TestRecord = <![CDATA[Public Record NameValue(Name ="", Value = 0.0)]]>.Value
        Dim result = GetGeneratedOutput(TestSourceCode, TestRecord)
        For Each diag In result.Diagnostics
            Assert.AreNotEqual(diag.Id, "BC42502", diag.ToString())
        Next
        Assert.AreEqual(GetHash(result.Output), "BC7C020F61E28086AD90B7E27DA8B21E")
    End Sub

    <TestMethod>
CyrusNajmabadi
@CyrusNajmabadi
yes. i get that. i'm saying: the test isn't meaningful. because if i get a different hash i have no way to ascertain if that's a good change in behavior, or an unwanted one.
the 'previous' result means nothing to me. no one can look at BC7C020F61E28086AD90B7E27DA8B21E and understand waht it means.
and if the new hash was different, having hte unit test say "you expected BC7C020F61E28086AD90B7E27DA8B21E, but got <something else>" isn't actionable.
Mohammad Hamdy Ghanem
@VBAndCs
I have another unit tests to test critical parts:

    <TestMethod>
    Public Sub WriteNameValue()
        Dim code = <![CDATA[(Name ="", Value = 0.0)]]>.Value

        Dim expected = <![CDATA[   <Key>
   <DefaultValue("1= #1/1/0001#")>
   Public Shadows Property [Date] As Date

   <Key>
   <DefaultValue("1=__QUOTE____QUOTE__")>
   Public Property [Name] As String

   <Key>
   <DefaultValue("1= 0.0")>
   Public Property [Value] As Double


]]>.Value

        Dim properties As New List(Of PropertyInfo)
        Dim result = WriteProperties(code, properties, "Inherits Test").Replace(vbCrLf, vbLf)
        Assert.AreEqual(result, expected)


        expected = <![CDATA[    Public Sub New(
                Optional [date] As Date = #1/1/0001#,
                Optional [name] As String ="",
                Optional [value] As Double = 0.0
            )

        Me.Date = [date]
        Me.Name = [name]
        Me.Value = [value]
    End Sub

]]>.Value

        Dim sb As New StringBuilder
        RecordParser.WriteConstructor(properties, sb)
        result = sb.Replace(vbCrLf, vbLf).ToString()
        Assert.AreEqual(result, expected)


        expected = <![CDATA[    Public Function [With](
                Optional [date] As Date? = Nothing,
                Optional [name] As [Optional](Of String) = Nothing,
                Optional [value] As Double? = Nothing
            ) As NameValue

        Return  New NameValue(
            If ([date].HasValue, [date].Value, Me.Date),
            If ([name].HasValue, [name].Value, Me.Name),
            If ([value].HasValue, [value].Value, Me.Value)
        )
    End Function

]]>.Value
        sb.Clear()
        RecordParser.WriteWith("NameValue", "", properties, sb)
        result = sb.Replace(vbCrLf, vbLf).ToString()
        Assert.AreEqual(result, expected)

    End Sub
CyrusNajmabadi
@CyrusNajmabadi
none of that addresses teh point i made about your test :)
if it fails. it's not actionable.
Mohammad Hamdy Ghanem
@VBAndCs
@Clockwork-Muse Any change in the generator that changes the output, will need to change all the tests expected values, using hashing or plain strings.
CyrusNajmabadi
@CyrusNajmabadi
Any change in the generator that changes the output, will need to change all the tests expected values
taht's not necessarily true
it will be true if you're doing an exact check of contents (which is up to you)
you could, for example, do a whitespace-insensitive check, if you want your tests to effectively say: i don't care if whitespace changes
the hashing approach is somewhat the worst of all worlds
Stephen A. Imhoff
@Clockwork-Muse

you could, for example, do a whitespace-insensitive check, if you want your tests to effectively say: i don't care if whitespace changes

Right, this

CyrusNajmabadi
@CyrusNajmabadi
it always changes whenever the output cahnges (no matter how slight). but it gives the least amoutn of information about what changed.
it soundsl ike you want hte opposite. you want slight changes in your output to to not cause assertion failures.
Stephen A. Imhoff
@Clockwork-Muse
Which is why I brought up the point about syntax tree checking, because often for generators you don't care what it looks like.
Mohammad Hamdy Ghanem
@VBAndCs

if it fails. it's not actionable.

If NameValue integral test fails, I will run WriteNameValue and see what is going on.

CyrusNajmabadi
@CyrusNajmabadi
I will run WriteNameValue and see what is going on.
why don't you just always run this then :)
they're tests :)
Mohammad Hamdy Ghanem
@VBAndCs
In fact I need these tests as a debugging entry points rather than tests.
CyrusNajmabadi
@CyrusNajmabadi
just have each test call your 'invariant checking test helper'
if you already have a test helper which can tell you precisely what is different... then that's what your tests should use. you shouldn't need a manual step fo going "oh, the hash changed, let me manually run this helper to find out why"
the tests can all run without manual intervention
Mohammad Hamdy Ghanem
@VBAndCs
I ran them all after every change. I will debug into some of them to see why things fail.
CyrusNajmabadi
@CyrusNajmabadi
...
i mean, you're proving the point :)
Mohammad Hamdy Ghanem
@VBAndCs
This helps me while updating the generator.
CyrusNajmabadi
@CyrusNajmabadi
you're picking a lossy testing strategy that requires you to have to debug in to figure out what went wrong when it fails.
let's step back.
i get yoru goal here. you want to be able to updat ethe generator, but not have to deal with lots of fallout when somethign changes
however the hashing approach does not provide that. indeed, it's worse. because any slight changes in input produces a new hash.
Mohammad Hamdy Ghanem
@VBAndCs
yes
CyrusNajmabadi
@CyrusNajmabadi
so... it doesn't meet that goal. and it actually costs you more because you can't do anything with the assertion failure. you have to then go in and debug it all and try to figure out if you have a bug, or if you need to update the test.
so. to meet your goal, what would be better would be to send your code not through a hashing step, but through a normalization step.
where your 'normalization' ensures that two strings with cosmetic differences (as defined by you) are treated the same
Mohammad Hamdy Ghanem
@VBAndCs
I can watch the output while debugging. See no issue here. This is much faster that parsing the syntax tree of the output and write long code to check nodes.
CyrusNajmabadi
@CyrusNajmabadi
then, if you update your generator, and you only make a cosmetic difference, your test doesn't fail.
My point is... if you do this, you don't need to watch anything in teh first place :)
your tests either pass, and everything is good. or they fail, and you know why.
This is much faster
in one case you hae to do work. in the other, the test harness does the work (likely in <1ms or 1mu-s per test) :)
Stephen A. Imhoff
@Clockwork-Muse

I can watch the output while debugging.

This is antithetical to good testing output. Your test report should clearly say "this specific thing is unexpected". For one thing, this enables compile/test runs on targets you can't interactively debug (for example, you can legally only develop code for a Mac on a Mac - I don't want to buy one if I can help it, so github actions for me)

Mohammad Hamdy Ghanem
@VBAndCs
I use tests as a helper tool not a goal. It did helped like this in this project. If I ran into troubles I will modify them as needed. Thanks.
CyrusNajmabadi
@CyrusNajmabadi
I use tests as a helper tool not a goal
Sure. the general point here is that there are more effective ways to use them (in terms of spending less time to get more value out of htem).