Canary Tests

Tsaukpaetra

@unperverted-vixen said in Canary Tests:

If you're doing anything with async void methods, convert them to return Tasks instead.

Hmm, good point, checking..

        public async Task<IPublicIPAddress> GetOrCreateIPAddress(string IPAddressName)
        {
            IPublicIPAddress result = await AzureClient.PublicIPAddresses.GetByResourceGroupAsync(CurrentResourceGroup, IPAddressName);
            if (result == null)
            {
                //Try to create it
                result = await AzureClient.PublicIPAddresses.Define(IPAddressName)
                    .WithRegion(CurrentRegion)
                    .WithExistingResourceGroup(CurrentResourceGroup)
                    .WithDynamicIP()
                    .CreateAsync(default(System.Threading.CancellationToken),false);
            }
            return result;
        }

I think the issues stopped when I added the default(System.Threading.CancellationToken),false parameters into the CreateAsync() call, but if there are exceptions I'm still not able to catch them.

Unperverted Vixen

@tsaukpaetra I can't find the signature for the method you're calling, so I'm not sure exactly what the .CreateAsync() parameters do...

So that method itself returns a task - you're awaiting whatever calls it, etc., all the way up the stack?

Tsaukpaetra

@unperverted-vixen said in Canary Tests:

@tsaukpaetra I can't find the signature for the method you're calling, so I'm not sure exactly what the .CreateAsync() parameters do...

So that method itself returns a task - you're awaiting whatever calls it, etc., all the way up the stack?

.... Awe sheate! Three levels up indeed it's returning void.

Working up the tree, somehow returning Task breaks Parallel.ForEach here:


// Loop through VirtualMachineBuildDTOs to make sure OnDeck counts Match the DB
                await Task.Run(() => Parallel.ForEach(CloudBuilds, BalanceVMs));

...

static async Task BalanceVMs(ComputeCloudBuild aVMBuild)

I'm assuming the Parallel class doesn't do sub-async things here, so I'll probably end up adjusting that line a bit...

Tsaukpaetra

@tsaukpaetra said in Canary Tests:

I'll probably end up adjusting that line a bit...

That was easier than I thought it would be. Turned:

await Task.Run(() => Parallel.ForEach(CloudBuilds,  BalanceVMs));

into

await Task.WhenAll(CloudBuilds.Select(x => Task.Run(() => BalanceVMs(x))));

Which (I think) will basically do the same thing... Probably.

Considering we only have one build definition in use anyways it probably won't make any difference....

cartman82

@lorne-kates said in Canary Tests:

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

.aspx.vb file:
lblText.Text = "<a href='" & someURL & ">Text"

TRWTF is webforms don't auto-escape that label text.

cartman82

The only one I can think of:

(in LESS or SASS file)

.my-button {
  background: #c4c4c4;
}

(there is probably a variable you should be using there, chump. Don't be lazy).

TheCPUWizard

@tsaukpaetra said in Canary Tests:

Which (I think) will basically do the same thing... Probably.

Glad you got that fixed [find those returns of void on an async can be tedious!]

The two implementations you posted will do very similar things with differences in scaling/slicing/etc. Since you are (apparently) not running large collection sizes you will probably never notice any differences.

If this technique is leveraged in other cases where there could be hundreds, thousands, or more items in the collections the differences could become important...

Lorne Kates

@tsaukpaetra said in Canary Tests:

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

If it's thASYNC'Dat up the yFUCKEDou be may

Lorne Kates

@cartman82 said in Canary Tests:

my

The use of "my" anywhere in the codebase. It's either a developer with cargo cult brain worms, or a blind copy pasta from Random Website.

izzion

@lorne-kates
Or mysql_this_code_works_really

TwelveBaud

@lorne-kates Or Perl, or VB.net, or...

Though I guess that doesn't rule that out.

Tsaukpaetra

@thecpuwizard said in Canary Tests:

@tsaukpaetra said in Canary Tests:

Which (I think) will basically do the same thing... Probably.

Glad you got that fixed [find those returns of void on an async can be tedious!]

The two implementations you posted will do very similar things with differences in scaling/slicing/etc. Since you are (apparently) not running large collection sizes you will probably never notice any differences.

If this technique is leveraged in other cases where there could be hundreds, thousands, or more items in the collections the differences could become important...

Yeah, there's really only slight differences in the VM builds themselves (like, what size (F1,F2,A8) it is), my current stretch goal is to decouple the build from the game server levels that are going to be instanced on them (since all builds use the same base image they all can run any given level).
I'm mostly there except I'm having trouble with projected-needs calculation (basically "I have this much capacity, I want to use this much more capacity, how many of what type of build should I create to satisfy the need"). Something tells me I should be using calculus for this, but all those modules got archived half a decade ago....

dkf

@lorne-kates said in Canary Tests:

The use of "my" anywhere in the codebase.

You prefer this? Or self?

Dreikin

@dkf said in Canary Tests:

@lorne-kates said in Canary Tests:

The use of "my" anywhere in the codebase.

You prefer this? Or self?

thisHereNoun and thatThereNoun, of course.

Dreikin

@tsaukpaetra said in Canary Tests:

@thecpuwizard said in Canary Tests:

@tsaukpaetra said in Canary Tests:

Which (I think) will basically do the same thing... Probably.

Glad you got that fixed [find those returns of void on an async can be tedious!]

The two implementations you posted will do very similar things with differences in scaling/slicing/etc. Since you are (apparently) not running large collection sizes you will probably never notice any differences.

If this technique is leveraged in other cases where there could be hundreds, thousands, or more items in the collections the differences could become important...

Yeah, there's really only slight differences in the VM builds themselves (like, what size (F1,F2,A8) it is), my current stretch goal is to decouple the build from the game server levels that are going to be instanced on them (since all builds use the same base image they all can run any given level).
I'm mostly there except I'm having trouble with projected-needs calculation (basically "I have this much capacity, I want to use this much more capacity, how many of what type of build should I create to satisfy the need"). Something tells me I should be using calculus for this, but all those modules got archived half a decade ago....

You might only need basic arithmetic. E.g., if you just want to make sure your build capacity the smallest number of servers that will meet or exceed your desired capacity, you basically fill with MAX_SIZE servers until the remaining needed capacity is <= MAX_SIZE_CAPACITY. Then you find the smallest that's still greater than what's needed. (I assume that if the problem was this simple you'd already have done that; I'm just demonstrating that as stated at least some solutions are pretty simple).

Tsaukpaetra

@dreikin said in Canary Tests:

@tsaukpaetra said in Canary Tests:

@thecpuwizard said in Canary Tests:

@tsaukpaetra said in Canary Tests:

Which (I think) will basically do the same thing... Probably.

Glad you got that fixed [find those returns of void on an async can be tedious!]

The two implementations you posted will do very similar things with differences in scaling/slicing/etc. Since you are (apparently) not running large collection sizes you will probably never notice any differences.

If this technique is leveraged in other cases where there could be hundreds, thousands, or more items in the collections the differences could become important...

Yeah, there's really only slight differences in the VM builds themselves (like, what size (F1,F2,A8) it is), my current stretch goal is to decouple the build from the game server levels that are going to be instanced on them (since all builds use the same base image they all can run any given level).
I'm mostly there except I'm having trouble with projected-needs calculation (basically "I have this much capacity, I want to use this much more capacity, how many of what type of build should I create to satisfy the need"). Something tells me I should be using calculus for this, but all those modules got archived half a decade ago....

You might only need basic arithmetic. E.g., if you just want to make sure your build capacity the smallest number of servers that will meet or exceed your desired capacity, you basically fill with MAX_SIZE servers until the remaining needed capacity is <= MAX_SIZE_CAPACITY. Then you find the smallest that's still greater than what's needed. (I assume that if the problem was this simple you'd already have done that; I'm just demonstrating that as stated at least some solutions are pretty simple).

Yeah, we have it working fine on a per-build basis where specific levels prefer a given build, just not the more generalized one. 📶

I'll probably just backburner it while we transition our level identifier from ip+port to levelname+instance number.

AlexMedia

@lorne-kates said in Canary Tests:

What's your Canary Test?

People handrolling JSON by doing lots of string concatenations without bothering to sanitise their values.

Tsaukpaetra

@alexmedia said in Canary Tests:

@lorne-kates said in Canary Tests:

What's your Canary Test?

People handrolling JSON by doing lots of string concatenations without bothering to sanitise their values.

You call them people? Brave...

Lorne Kates

@tsaukpaetra said in Canary Tests:

@alexmedia said in Canary Tests:

@lorne-kates said in Canary Tests:

What's your Canary Test?

People handrolling JSON by doing lots of string concatenations without bothering to sanitise their values.

You call them people? Brave...

Not brave, just smart. Police get suspicious when you know to call them "corpses" .

AlexMedia

@lorne-kates

To make matters worse, it's everywhere. Handrolled JSON in views, returned from controllers, AsJSONString(); in interfaces, and so on.

Lorne Kates

Oh god it's going to be one of those days working with code isn't it?

Layers of canary-gas here...

did he not realize you can re-use the "tempItem" variable? Does he not understand OOP? Did he thing that saying tempItem = new CUSTItem() would "overwrite" the previous item (that is in the cart line collection now) *
Did he really need four instances of tempItem with increasing names of tempItem2, tempItem3? Because he's declaring those items inside the scope of the If-End If block. He could have called them all tempItem and there'd be no problem. Does he not know about scope levels? **
Oh, and this means not only did he copypasta his code 4 times-- he then had to modify the code 4 times. The "redacted code" just populates that item with a string related to the CustomProperty, sets the price based on the CartLine price. That's it. So he could have a function called CUSTCartLine GetTempItem(string fakeItemNo, decimal linePrice), instead of copypasting the code multiple times. Does he not know about re-usability? ***

* - inb4 "obviously he doesn't know"
** - inb4 "obviously he doesn't know"
*** - inb4 "obviously he doesn't know"

		'Small Order fee
		If Order.CustomProperty("smallOrderFee") <> "" Then
				Dim tempItem As CUSTCartLine = New CUSTCartLine
' redacted code here
				tempCartLineList.Add(tempItem)
			End If
		End If

		'Personal Shipping Fee
		If Order.Shipment.CustomProperty("PersonalShippingFee") <> "" Then
				Dim tempItem2 As CUSTCartLine = New CUSTCartLine
' redacted code here
				tempCartLineList.Add(tempItem2)
			End If
		End If

		'Rush Order Price
		If Order.CustomProperty("RushOrderPriceBasedLCY") <> "" Then
				Dim tempItem3 As CUSTCartLine = New CUSTCartLine
' redacted code here
				tempCartLineList.Add(tempItem3)
			End If
		End If

		'Shipping fee
		If Order.Shipment.ShippingCharge > 0 Then
			Dim tempItem4 As CUSTCartLine = New CUSTCartLine
` redacted code here
			tempCartLineList.Add(tempItem4)
		End If

hungrier

@Lorne-Kates Not only that but he's writing VB code in PHP

iKnowItsLame

@cartman82 said in Canary Tests:

SASS

Since this thread has already been necro'd:

wtf1 (?): using sass
wtf2 (definitely): using $vendor-orange as name

... so one ends up with "$vendor-orange: #008080"

/teal is the new orange?
//sounds like a S01 on netflix

Captain

@Lorne-Kates said in Canary Tests:

[Object object]. If you ever see that anywhere, someone's fucked up the javascript.

Meh I have one of those in a branch I'm actively working on. Yeah, the JS is fucked up. Still a meh as a universal warning of suckage.

Lorne Kates

@Captain said in Canary Tests:

@Lorne-Kates said in Canary Tests:

[Object object]. If you ever see that anywhere, someone's fucked up the javascript.

Meh I have one of those in a branch I'm actively working on. Yeah, the JS is fucked up. Still a meh as a universal warning of suckage.

"Oh, that only counts for OTHER developers, not me, nuh-uh."

Captain

@Lorne-Kates I mean, it only counts if you commit code that way, more than once or twice. Otherwise, it's pretty normal to mess up a type and forget to call a method on an object. That's why programmers like type systems and testing and stuff. So you can catch the dumb mistakes.

I thought this thread was about anti-patterns so misguided you wonder if they even used the language before.

Lorne Kates

@Captain said in Canary Tests:

I thought this thread was about anti-patterns so misguided you wonder if they even used the language before.

Lorne Kates quotes @Captain :

it only counts if you commit code that way

Lorne Kates

@levicki said in Canary Tests:

@Lorne-Kates If you need to know all that then it's easier to write HTML by hand

Tweet tweet, motherfucker.

dkf

@Captain said in Canary Tests:

So you can catch the dumb mistakes.

Only some of them. Where there's a way, there's a dumb way.

PJH

@levicki said in Canary Tests:

@Lorne-Kates If you need to know all that then it's easier to write HTML by hand.

@PJH said in Canary Tests:

Ghod gave us INET_ADDRSTRLEN and PATH_MAX for a reason.

It's not like INET_ADDRSTRLEN is going to change anytime soon, it's a fucking constant, using #define is more portable than pulling a header not available on every platform.

1) What do you think brings INET_ADDRESTRLEN into scope?

B) If INET_ADDRSTRLEN isn't in a header available on your platform, then I suggest that neither is any of the other socket related stuff that would be using it.

As for PATH_MAX that is again Linux only and is defined as 4096 characters including terminating \0. On Windows it is called MAX_PATH and it's defined as 260 characters (out of which 4 are drive letter, colon, backslash and \0, so actual path can be 256 characters) unless you are prefixing it with \\?\ in which case it can be 32767 characters. So, if you want cross-platform code anything you put there is wrong and 256 as used in your example is at least sane assumption for most platforms.

Doesn't address my underlying issue of not using a defined constant (or some sort of filepath handing library) in those situations and simply using a fixed 'random' number for the length, (typically far short of whatever PATH_MAX or MAX_PATH are.)

If you "know" that the filename will never exceed 256 characters (640Mb anyone?) then stick it in a define in a header with a comment near it to that effect. Don't simply have 256 dotted all over the place.

I still maintain that both of those examples are 'canaries' of badly thought out/written code, or at least significant ignorance of what they're trying to do/use.

dkf

@PJH said in Canary Tests:

I still maintain that both of those examples are 'canaries' of badly thought out/written code, or at least significant ignorance of what they're trying to do/use.

Magical constants without names, especially outside of tests, are indicative of problems.

PJH

@dkf said in Canary Tests:

Magical constants without names, especially outside of tests, are indicative of problems.

Appearing more than once, doubly so.

PleegWat

@levicki said in Canary Tests:

As for PATH_MAX that is again Linux only and is defined as 4096 characters including terminating \0.

Isn't that also filesystem dependent? The 4kb may well apply to NFS, but I'm pretty sure in ext2/3/4 the only limit is on path components.

dkf

@PleegWat said in Canary Tests:

I'm pretty sure in ext2/3/4 the only limit is on path components

You end up hitting other limits, such as buffer sizes in the kernel. I've no idea what they'd be doing there, but it's truly not safe to assume you've got unbounded space available.

And 4kB is enough for a very long filename, even with quite a bit of symlink shenanigans. The longest I've found on this system with a quick search is 370 characters…

Captain

See I just smashed the [Object object] I was talking about, and it's another feature ported over to my new toolchain.

Zecc

@dkf said in Canary Tests:

Magical constants without names, especially outside of tests, are indicative of problems.

Sigh.

Program that exports CSV files:

SELECT * FROM SomeTable;

Program that reads CSV files (all comments mine. The only comments in this codebase that are not old versions of code are mine¹):

string fileContent = File.ReadAllText("export.csv");
string[] fileLines = fileContent.Split('\n');
foreach(string line in fileLines)
{
    RedactedClass cls = new RedactedClass();
    try {
        string[] a = line.Split(';');
        // Some field values actually have ; in them if you're wondering.
        // The program that does the export is smart enough to surround those in quotes.
        // It's not smart enough to escape quotes though. 😒
        if (a.Length <= 20)
        {
            cls.Id = a[0];
            cls.Name = a[1];
            cls.RedactedProperty[2];
            // ...
            // repeat for many other properties that end up not being used anywhere else in the entire codebase
            // ...
            cls.AnonymizedProperty = a[19];
        }
        else {
            cls.Id = a[0];
            cls.Name = a[1];
            cls.RedactedProperty[2];
            // ...
            // repeat for many other properties that end up not being used anywhere else in the entire codebase
            // ...
            cls.AnonymizedProperty = a[19];
            // ...
            // oh god, this initializes 30 or so more properties no one cares about
        }
    }
    catch(Exception exc)  // "unused variable" error here
    {
        cls.Error = true;  // I love this pattern 🤦
    }
}

¹ My predecessor used full-stack version "control": // + WinRar + NTFS + FTP

Edit to clarify: cls was used. The code wasn't that bad.

Zecc

Also in this codebase:

Lists initialized with zero capacity.
Dispose method in classes that do not implement IDisposable (see below).
Class named Globals. 'Nuff said.

Not in this code (not before I got here, anyway):

using
finally
Static classes.
Interfaces. Literally. Not a single interface.
Class inheritance. Not even once. All specialization, including properties, was done by adding to the would-be base class.

Lorne Kates

@Captain said in Canary Tests:

See I just smashed the [Object object] I was talking about, and it's another feature ported over to my new toolchain.

Does that include a re-implementation of the scroll bar?

Unperverted Vixen

@Zecc said in Canary Tests:

Also in this codebase:

Lists initialized with zero capacity.

Looks dumb, but the source says it's functionally identical to not passing in a length. Could be worse.

Not in this code (not before I got here, anyway):

using

finally

Ouch. You can live without one, or the other, but not both.

Interfaces. Literally. Not a single interface.

If you have an older codebase (one predating the using statement being in the language), that's not a big surprise. Bolting on DI is a lot of work.

Class inheritance. Not even once. All specialization, including properties, was done by adding to the would-be base class.

Captain

@Lorne-Kates said in Canary Tests:

See I just smashed the [Object object] I was talking about, and it's another feature ported over to my new toolchain.

Nope. The logic was correct, I just had a type error where I was binding to my Knockout view.

dkf

@Zecc said in Canary Tests:

Not in this code (not before I got here, anyway):

Class inheritance. Not even once. All specialization, including properties, was done by adding to the would-be base class.

That's where you should be thinking of hitting the previous developer with a rolled up newspaper, like a puppy that's chewed up your best chair.

PJH

@levicki said in Canary Tests:

And it's actually defined as:
#define INET_ADDRSTRLEN 22

PleegWat

@levicki said in Canary Tests:

I guess they don't need the port numbers then.

It's called an internet address. Ports are TCP, not IP.

PJH

@PleegWat said in Canary Tests:

@levicki said in Canary Tests:

I guess they don't need the port numbers then.

It's called an internet address. Ports are TCP, not IP.

IP addresses are OSI layer 3 (network) Ports are generally (and specifically what we're talking about) found, where they exist, on layer 4(transport) [used with TCP and UDP. ICMP, which is also there, doesn't have the concept of ports. See /etc/protocols for more pendants, such as ipv6 over ipv4.]

What I found puzzling is what Windows is expecting in the other 6 bytes, since inet_addrstrlen need only be 16.

Unless they're mistakenly expecting room for a : and a port number...?

Which really would be , since anyone writing to that won't be writing portable code.

INET6_ADDRSTRLEN (which should be 46) is also weird at 65.

PJH

@levicki said in Canary Tests:

When in doubt, RTFM:

Which wasn't easily found, hence the link I did provide.

So then, along with a touch of xkcd://standards, since they're decided stuffing two disparate things, into something that previously only contained one, was a good idea.

I personally prefer having well-defined API to do string conversions rather than having to roll my own.

Because inet_pton() was either not complicated enough, or NIH. Possibly a bit of both.

@levicki said in Canary Tests:

Also, handling address and port separately when you almost always need both is just retarded.

I don't see the web littered with URLs containing :80 or :433, so I'm going to call "almost always" somewhat exaggerated.

Tsaukpaetra

@PJH said in Canary Tests:

I don't see the web littered with URLs containing :80 or :433, so I'm going to call "almost always" somewhat exaggerated.

Oh, they just moved the 80 to be http: and the 443 to https:. Just because a default has been communally adopted doesn't mean the underlying technology suddenly doesn't need it.

PleegWat

@Tsaukpaetra said in Canary Tests:

@PJH said in Canary Tests:

I don't see the web littered with URLs containing :80 or :433, so I'm going to call "almost always" somewhat exaggerated.

Oh, they just moved the 80 to be http: and the 443 to https:. Just because a default has been communally adopted doesn't mean the underlying technology suddenly doesn't need it.

You are conflating addresses and URIs. This also depends on which level you are working on.
If you are writing client code which communicates over HTTP or HTTPS, you are almost certainly working with URIs and this discussion doesn't apply.
If you are working with web server code in an existing web server environment (EG apache, nginx) then the (listen) addresses are handled in the server configuration and not in your application code at all.

Only if you are actually dealing with raw TCP or UDP sockets you will be dealing with this. And here my first suspicion is that there is an API level difference between windows and linux.
As far as I remember on linux, you always pass in the binary forms of these addresses, and the IP address and TCP port go into different places in the argument structs, or in different function calls altogether. The application code operating at that level will be handling the two separately.
I do not have a clue how this works on windows, but it is well possible the standard library there expects the addresses in string form, meaning a single buffer for both is more likely.

I'm not sure about any of this though - I have never needed to deal with this on windows, and while I work with low-level network code on linux pretty frequently, our sockets there are opened with device names as arguments.

dkf

@PleegWat said in Canary Tests:

on linux, you always pass in the binary forms of these addresses, and the IP address and TCP port go into different places in the argument structs

This. That's 16 bytes for the address (and 2 for the port for relevant protocols) for a v6 address, and 4 bytes (plus 2) for a v4 address. These are fixed binary widths, and you see them in several syscalls (especially bind()).

The hostname and protocol name parsing routines (which are a client library) have different limits.

Tsaukpaetra

@PleegWat said in Canary Tests:

You are conflating addresses and URIs

I'm not the one who suggested this, talk to @PJH .

PJH

@Tsaukpaetra said in Canary Tests:

@PJH said in Canary Tests:

I don't see the web littered with URLs containing :80 or :433, so I'm going to call "almost always" somewhat exaggerated.

Oh, they just moved the 80 to be http: and the 443 to https:. Just because a default has been communally adopted doesn't mean the underlying technology suddenly doesn't need it.

Not quite. http://www.example.com:443 and https://www.example.com:80 are both perfectly cromulent (if somewhat obfuscatory) URLs.

The bit after the last colon tells you the port, the bit before the first colon tell you what protocol you use over that port (said protocols having defaults of 80 and 443.)