Canary Tests


  • Trolleybus Mechanic

    Sometimes, you come across a certain pattern of code that makes you go WTF. Though, not because of that one piece of code itself. Sure, the piece of code IS a WTF, but TR :wtf: is that the code itself is just an indicator that there's so much more lurking underneath. It's such a fundamental misunderstanding of basic concepts, that you know you're going to find so, so much more as soon as you start digging. It's a warning sign.

    For me, it's this:

    .aspx file:

    <asp:label runat="server" id="lblText" />
    

    .aspx.vb file:

       lblText.Text = "<a href='" & someURL & ">Text</a>"
    

    The reason is because it shows an absolute lack of understanding of the tools at hand. For those not familiar with ASP.Net, a label is a managed control that renders out to a <span> with the contents being the .Text property. Directly into HTML. So better hope someURL is a valid URL, because there's no checking of any sorts.

    ASP also has a control <asp:HyperLink> with a .Text property and a .NavigateURL property, which render out to an <a> tag with href set with the properly escaped and encoded NavigateURL.

    So the programmer has bypassed the correct tool for the job, and grabbed one that will work if you put a lot more effort into it-- but still won't work 100% and can bite you hard. And sure enough, it revealed a deeper issue.

    • The Label control can either by a hyperlink to a product page, or can be a static span that displays a price, or can be a hyperlink that says "More Details" to a different page
    • The Label is then hidden if the user doesn't have permission to view pricing

    So if the Label is a "More Details", but the user doesn't have permission to "View Pricing", the label is hidden, which is a bug.

    If the programmer had used a proper hyperlink control for the hyperlink, and a static label for the pricing (two controls), they could have shown/hid the information properly because there are more than two states at play.

    And I see this abuse of <asp:Label> over and over and over. And it ALWAYS means trouble. Enough that I can apply it as a test to accurately predict how bug-riddled the code will be.

    So-- what is it for you? What's your Canary Test?

    fake edit also, entering tags is broken. Will have to file a bug.



  • <?php


  • ♿ (Parody)

    @lorne-kates said in Canary Tests:

    also, entering tags is broken

    Probably needed to select a category. That always gets me, too.


  • Trolleybus Mechanic

    @boomzilla said in Canary Tests:

    @lorne-kates said in Canary Tests:

    also, entering tags is broken

    Probably needed to select a category. That always gets me, too.

    Yes, I have to (see my bug). There is literally no reason why that should be the case, though. Aside from NodeBB "testing".



  • @lorne-kates The only thing I can think of is category-specific restrictions on tags. Which then if you can't enter tags without the category, that needs to be conveyed to the user before they try to enter tags.


  • I survived the hour long Uno hand

    @lorne-kates said in Canary Tests:

    So better hope someURL is a valid URL, because there's no checking of any sorts.

    First thing I noticed -- quotes are fucked up. I noticed this because I've been fixing code like this for weeks now. Thanks @arantor



  • So many different things as Canary Tests....but sometimes things may not be what they seem and could have [or had at a previous point in time] some value.

    Obviously I only have the information to go on in the post, but I can imagine:

    1. At some time there was the need for setting Text and Visibility.
    2. Later a requirement was added to have a hyperlink.

    Following the OCP from SOLID the goal would be to add items without changing the existing code - so this could make sense.

    Conversely, if the concept had been encapsulated from the beginning than a different implementation could have been swapped in [DIP] when the requirement was added. This approach has a lot of potential value, but is rarely seen (at least with any level of consistency)


  • Considered Harmful

    I don't do anything remotely complicated, but here's my usual canary warning:
    Proper code:

    @Inject @DefaultConfig(sharedRoot = true) Path path;
    //...
    asset.copyToFile(path);
    

    Canary:

    @Inject @ConfigDir(sharedRoot = false) File file;
    //...
    File confFile = new File(file + "config.conf");
    asset.copyToFile(confFile.toPath());
    

    Not bad on its own, if you intended to do anything complex, but if you only have one file, and you never touch the injected variable again, then you clearly didn't even read the documentation (not to mention being bad with Files). I see this WAY too commonly.


  • Trolleybus Mechanic

    This shit right here:

    https://i.imgur.com/UoeKEAH.png

    1. Randomly generated css class names. Means there's a css compiler at work. Means the css is an ungodly pile of unmanagable shit. Means someone in charge is so technically ignorant they think "omg we can obfuscate our css names and can't be hacked!"
    2. [Object object]. If you ever see that anywhere, someone's fucked up the javascript.

    OH AND LOOK! The page is absolutely unusable. The divs are all inline so they're going off the page. And the second div is actually UNDER the sidebar, so I can't click it. And since the class names are all random, and there's no ids on anything, I can't even fix it with a stylesheet.

    Maybe this works in "latest greatest gee-whiz bang" browser, but if your css is so fragile that you can fuck up a single column list of divs with it-- then you are doing CSS wrong.

    Fucking Patreon.

    https://i.imgur.com/TULg9yV.png


  • Notification Spam Recipient

    Red flag from yesterday:

    try
    {
        //do work
        //...
        //do some more
    }
    catch
    {
        throw;
    }
    

    And sure enough, it gets worse further.


  • Discourse touched me in a no-no place

    @mrl That doesn't get better if they log it on the way through. Exceptions are not like wild animals; catch-log-and-release is a bad approach!


  • Notification Spam Recipient

    @dkf said in Canary Tests:

    @mrl That doesn't get better if they log it on the way through. Exceptions are not like wild animals; catch-log-and-release is a bad approach!

    No worries, it's not logged anywhere, it gets swallowed.



  • @mrl said in Canary Tests:

    @dkf said in Canary Tests:

    @mrl That doesn't get better if they log it on the way through. Exceptions are not like wild animals; catch-log-and-release is a bad approach!

    No worries, it's not logged anywhere, it gets swallowed.

    My rule is that if someone threw it (up), you shouldn't swallow it. You are allowed to complain about it (or log it) but you must also clean up.

    The very yucky image that comes to mind is a very good way to enforce that rule.



  • @dkf said in Canary Tests:

    catch-log-and-release is a bad approach!

    I can be... However the information that triggered the exception is a distinct beast from the code that will eventually handle the exception (even if that code is the built in terminate-application).


  • Notification Spam Recipient

    Since I already posted a gem from this codebase, here's another one.

    var builder = new StringBuilder();
    var someBusinessObject = new SomeBusinessObject();
    var someManagerOrSth = GetManagerOrSth();
    
    try
    {
        //do work
        //...
        //do some more
    }
    finally
    {
        builder = null;
        someBusinessObject = null;
        someManagerOrSth = null;
    }
    


  • @mrl This isn't a WTF, it's a breakpoint insertion point. Which means someone has actually been doing some debugging work there. Whether they actually corrected the bug they were investigating remains to be seen.


  • Notification Spam Recipient

    @medinoc said in Canary Tests:

    @mrl This isn't a WTF, it's a breakpoint insertion point.

    Believe me, it's not.


    <Edit>
    Evidence:

    try
    {
        //do work
        //...
        //do some more
    }
    catch
    {
    
    }
    finally
    {
    
    }
    //----------------------------------
    try
    {
        //do work
        //...
        //do some more
    }
    finally
    {
        //do stuff having nothing to do with the try block
    }
    

    I rest my case.



  • @mrl This isn't what you first posted. What you first posted had a throw; in the catch block. What you just posted is a genuine red flag.


  • Notification Spam Recipient

    @medinoc said in Canary Tests:

    What you first posted had a throw; in the catch block.

    Which is a red flag if I ever seen one.


  • kills Dumbledore

    @mrl It's often used to have somewhere to insert a break point and investigate the exception closer to the source.

    It is a red flag to have it checked in to source control though. That sort of thing should be taken out when you've fixed or properly handled the throw


  • Notification Spam Recipient

    @jaloopa said in Canary Tests:

    @mrl It's often used to have somewhere to insert a break point and investigate the exception closer to the source.

    What's the point in that? Execution stops at catch block, you can see the exception, but not where it originated.
    Without the try block at all you can see the exception and you land at originating spot.



  • @mrl said in Canary Tests:

    What's the point in that? Execution stops at catch block, you can see the exception, but not where it originated.
    Without the try block at all you can see the exception and you land at originating spot.

    The last part is controlled by your exception settings, and may not do what you want...

    Consider an application where FooException is regularly thrown [there are many places within the framework / BCL where exceptions of a given type are thrown and caught under normal circumstances]. However I only care [i.e. Want a Breakpoint] if the exception is thrown from within a given context....



  • @jaloopa said in Canary Tests:

    It is a red flag to have it checked in to source control though. That sort of thing should be taken out when you've fixed or properly handled the throw

    Have you looked at the difference in IntelliTrace between the two? You may find that it is actually helpful in production [in which case it better be checked into source control!]


  • Notification Spam Recipient

    @thecpuwizard said in Canary Tests:

    @mrl said in Canary Tests:

    What's the point in that? Execution stops at catch block, you can see the exception, but not where it originated.
    Without the try block at all you can see the exception and you land at originating spot.

    The last part is controlled by your exception settings, and may not do what you want...

    Consider an application where FooException is regularly thrown [there are many places within the framework / BCL where exceptions of a given type are thrown and caught under normal circumstances]. However I only care [i.e. Want a Breakpoint] if the exception is thrown from within a given context....

    I can imagine this being a good idea in some special situations, ok. This is not one of them. Plus it should never get pushed to the repo.

    @thecpuwizard said in Canary Tests:

    @jaloopa said in Canary Tests:

    It is a red flag to have it checked in to source control though. That sort of thing should be taken out when you've fixed or properly handled the throw

    Have you looked at the difference in IntelliTrace between the two? You may find that it is actually helpful in production [in which case it better be checked into source control!]

    Helpful how?



  • @mrl said in Canary Tests:

    Helpful how?

    IntelliTrace in production is helpful in many ways.... Here is an oldie but goodie: https://blogs.msdn.microsoft.com/devops/2012/03/16/running-intellitrace-on-applications-in-production/


  • Trolleybus Mechanic

    @jaloopa said in Canary Tests:

    break point and investigate the exception

    Didn't your ide have "break on exception"?



  • @lorne-kates said in Canary Tests:

    Didn't your ide have "break on exception"?

    Did you not read why that is something completely different and may not be nearly as effective?


  • Trolleybus Mechanic

    @thecpuwizard said in Canary Tests:

    @lorne-kates said in Canary Tests:

    Didn't your ide have "break on exception"?

    Did you not read why that is something completely different and may not be nearly as effective?

    So break as soon as the exception is caught and then step your way up to observe what changes in the exception handling.


  • Discourse touched me in a no-no place

    @thecpuwizard said in Canary Tests:

    I can be... However the information that triggered the exception is a distinct beast from the code that will eventually handle the exception (even if that code is the built in terminate-application).

    True, but if you log the exception at every point where it passes through (an easy rule to implement) you get even trivial recoverable failures dumping multiple megabytes of crap into the log. That's a performance hog in production, and ends up obscuring those failures that really indicate a significant problem; the information might be in the logs, but so is everything else so you'll be lucky to notice that there's something that needs action.

    Overlogging isn't as bad as underlogging (and there's usually a decent sized sweet spot between the two extremes) but it is still a terrible problem.


  • FoxDev

    @medinoc said in Canary Tests:

    @mrl This isn't what you first posted. What you first posted had a throw; in the catch block. What you just posted is a genuine red flag.

    the first one was too. you don't need to do a catch throw to put a breakpoint insertion on. you enable break on throw or whatever your IDE calls it to break into the debugger when an exception is thrown.

    using catch {throw;} to put a breakpoint on just shows complete ignorance of your debugger. 🍊





  • @accalia said in Canary Tests:

    to put a breakpoint on just shows complete ignorance of your debugger

    [code]

    private static void Foo(int x)
        {
            for (int i=0; i<x; ++i)
            {
                try
                {
                    throw new FileNotFoundException();
                }
                catch
                {
                    // This is normal ignore it...
                }
            }
            throw new FileNotFoundException(); // This is important...
        }
        private static void F(int x)
        {
            Foo(x);  // Break into debugger if this call to Foo throws...
        }
    
        private static void G(int x)
        {
    
            Foo(x);
        }
    
        static void Main(string[] args)
        {
            for (int i = 0; i < 10; ++i)
            {
                try
                {
                    if (i % 2 == 0) F(i);
                    else G(i);
    
                }
                catch (FileNotFoundException e)
                {
                    // Handle the exception...
                }
            }
        }
    

    [/code]


  • Discourse touched me in a no-no place

    @lorne-kates said in Canary Tests:

    lblText.Text = "<a href='" & someURL & ">Text</a>"
    

    I spy a missing ' there.

    I do hope that wasn't a direct copy/paste...


    Anyway. C.

    if (p){
       free(p);
    }
    

    Usually an indicator that they don't know what they're doing, or enough about the language they're using.

    Also in this category:

    #define IP4_LEN 16
    

    and

    char filename[256];
    

    Ghod gave us INET_ADDRSTRLEN and PATH_MAX for a reason.


    int *p;
    [... some time later ...]
    p = malloc(sizeof int);
    

    Yeah - I really need to go around changing more stuff than I need to when the type of p changes. Don't you know about p = malloc(sizeof *p)?



  • FoxDev

    @medinoc said in Canary Tests:

    @accalia, cf Post #22

    Conditional exceptions are a thing, and if your application regularly throws exceptions then you're probably doing things very very very wrong.

    do you have ANY idea just how much overhead exceptions have in their construction AND handling? it's a lot. Like REALLY a lot. you should really avoid exceptions in the first place whenever possible.

    @thecpuwizard said in Canary Tests:

    [code]

    yes, that's code. now what does it mean?


  • Trolleybus Mechanic

    @pjh said in Canary Tests:

    I spy a missing ' there.

    Not a copy/paste. But the missing ' is yet another reason not to do shit like this.



  • @accalia said in Canary Tests:

    if your application regularly throws exceptions then

    While I agree, if the framework does this, you are basically powerless [unless you abandon the language/platform]

    As far as the code, there were comments about where it was desired to set a breakpoint so that the one location was the only time the debugger was invoked.

    I agree conditional breakpoints can be used in many places. Unfortunately experience has shown that the vast majority do not know how o use them at all (let alone effectively).


  • FoxDev

    @thecpuwizard said in Canary Tests:

    I agree conditional breakpoints can be used in many places. Unfortunately experience has shown that the vast majority do not know how o use them at all (let alone effectively).

    in other words my comment prior:

    @accalia said in Canary Tests:

    using catch {throw;} to put a breakpoint on just shows complete ignorance of your debugger.



  • @accalia said in Canary Tests:

    in other words my comment prior:

    Is something I still disagree with. There are a few debuggers that I would claim to be expert on [I would mention that I worked with the development team on them, but that would set Blakeyrat off on another rant :) ].

    Knowledge of the debugger is part of the equation in determining the most effective means of dealing with specifics. For .NET there are cases where a conditional breakpoint cannot achieve the desired goals.



  • @accalia said in Canary Tests:

    you enable break on throw or whatever your IDE calls it to break into the debugger when an exception is thrown.

    And then you manually step through a bunch of HttpExceptions as IIS starts up. And then a bunch of TargetInvocationExceptions since a (common) NuGet package sucks at doing reflection while processing its Web.config settings. And then a few AggregateExceptions for good measure. And miss the actual failed page load, failed service injection, or failed TPL call you actually want to debug. You can't turn on "break on throw, but only if a certain execution path is on the stack".


  • FoxDev

    @twelvebaud But you can tell the debugger what exceptions to look out for.



  • @raceprouk said in Canary Tests:

    But you can tell the debugger what exceptions to look out for.

    Sure, try to catch the 117th TargetInvocationException out of the 200 thrown...just by counting how many times you have hit continue....


  • FoxDev

    @thecpuwizard I think you can also say to only break on exceptions in your own code, not in the framework.



  • @raceprouk said in Canary Tests:

    think you can also say to only break on exceptions in your own code, not in the framework.

    Except that the example provided is where the exception is thrown inside the framework...

    If the throw was in your own code, then 3 breakpoints with sequential triggering would do the trick, but one can not readily do this when the source code for all of the points is not accessible.....



  • @accalia said in Canary Tests:

    @medinoc said in Canary Tests:

    @mrl This isn't what you first posted. What you first posted had a throw; in the catch block. What you just posted is a genuine red flag.

    the first one was too. you don't need to do a catch throw to put a breakpoint insertion on. you enable break on throw or whatever your IDE calls it to break into the debugger when an exception is thrown.

    using catch {throw;} to put a breakpoint on just shows complete ignorance of your debugger. 🍊

    It could also be a workaround. I have seen where break on throw drops you into the middle of a library that produced the exception. Usually you would actually want to be in your own code when the debugger halts execution.


  • Winner of the 2016 Presidential Election

    @medinoc said in Canary Tests:

    @accalia, cf Post #22

    FTLFY.


  • Notification Spam Recipient

    @mrl said in Canary Tests:

    @jaloopa said in Canary Tests:

    @mrl It's often used to have somewhere to insert a break point and investigate the exception closer to the source.

    What's the point in that? Execution stops at catch block, you can see the exception, but not where it originated.
    Without the try block at all you can see the exception and you land at originating spot.

    Unless you have something upstream that's catching exceptions higher up in the rabbit hole.

    Case in point: our Master Server application can occasionally fail to refresh information from Azure. My predecessor decided to just try catch at the top of the loop.

    Today I was troubleshooting why occasionally the program thinks a VM is experiencing Schrodinger's Paradox.

    I took out that stupid catch and.... Nothing. The issue occurred about six layers of async down and up longer had any context better than "reference not set to an instance of an object in [External code]".

    Only after careful dismemberment and hacking around splattering try catches everywhere (because for some reason the exception wasn't getting caught and stepping through made no difference) I found out that apparently sometimes the Azure Fluent Client thing can't find a Public IP address object it just created. Because reasons. Which sucks because it really did create it, but since that's the first step to creating a VM it aborts and tries again, but with a new IP address object. This inevitably lead to the resource group becoming inundated with unassigned IP address objects. Woohoo...

    I have no idea where I was going with that...


  • Notification Spam Recipient

    On that note, anyone have any idea why a try-catch handler wouldn't actually catch an exception? It was basically just aborting the thread but not actually aborting?

    Really weird situation I've never seen before ever...


  • And then the murders began.

    @tsaukpaetra said in Canary Tests:

    On that note, anyone have any idea why a try-catch handler wouldn't actually catch an exception? It was basically just aborting the thread but not actually aborting?

    Really weird situation I've never seen before ever...

    Generally I think that means there was async of some sort involved...


  • Notification Spam Recipient

    @unperverted-vixen said in Canary Tests:

    @tsaukpaetra said in Canary Tests:

    On that note, anyone have any idea why a try-catch handler wouldn't actually catch an exception? It was basically just aborting the thread but not actually aborting?

    Really weird situation I've never seen before ever...

    Generally I think that means there was async of some sort involved...

    @tsaukpaetra said in Canary Tests:

    The issue occurred about six layers of async down

    Yeah. We're deep in async hell in this application...


  • And then the murders began.

    @tsaukpaetra Oh. I didn't realize those were connected. :facepalm:

    As I understand it, the two big things to make sure exceptions get passed up the chain properly are:

    1. If you're doing anything with async void methods, convert them to return Tasks instead.
    2. Don't "fire and forget", make sure you always await a result.

Log in to reply